Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rfc: Proposal for a way to disambiguate shadow parts when composing components #6523

Closed
KingOfTac opened this issue Nov 17, 2022 · 5 comments
Labels
status:triage New Issue - needs triage

Comments

@KingOfTac
Copy link
Collaborator

KingOfTac commented Nov 17, 2022

💬 RFC

Recently it has come up that a potential shortcoming in Foundation templates is the common usage of the same part names across components and trying to style template parts that are deeply nested in composition scenarios. The problems and solution illustrated here tend to break the isolation of Shadow DOM, however can prove to be useful for library authors who want to provide more flexibility for styling certain components.

🔦 Context

To illustrate the problem, let's look at a custom template that composes more than one Foundation component that both have a part named control.

const myElTemplate = html`
  <my-text-field></my-text-field>
  <my-button></my-button>
`;

In some situations in document styles, or if MyEl is composed into another component you may need to reach into MyEl and tweak the styles on the composed components' control parts.

Currently there are two ways that this can be accomplished:

  1. Use CSS Custom Properties, or DesignTokens to apply the styles to the composed components. This works for one off scenarions but quickly becomes bloated when you need to support multiple properties for external styling.

  2. Use the exportparts global attribute. This works to expose the entire part element for external styling, however this is where we run into the issue of two components using the same part name. Let's take a look at myElTemplate using exportparts on both nested components.

const myElTemplate = html`
  <my-text-field exportparts="control"></my-text-field>
  <my-button exportparts="control"></my-button>
`;

We can now do something like my-el::part(control), however since we are exporting control from both the nested components, this would style both control parts. So this brings into question how we would tell the difference when only needing to style one of the elements.

💻 Examples

The recent template changes to support the future template builder help enable the proposed solution in a relatively low friction way.

Let's take Button's template for example:

// I'm only including a part of the template for brevity
function buttonTemplate<T extends FASTButton>(
  options: ButtonOptions = {}
): ElementViewTemplate<T> {
  return html<T>`
    <button
      class="control"
      part="control"
    >
    </button>
  `;
}

Given that an options object is passed in, we can extend it to enable disambiguation of the part name. Ideally this should be done in a way that doesn't clash with the upcoming template builder or done as a part of that work.

type ButtonOptions = StartEndOptions & {
  shadowPartNames: {
    button: string
  }
}

const defaultButtonOptions: Partial<ButtonOptions> = {
  shadowPartNames: {
    button: 'control'
  }
}

function buttonTemplate<T extends FASTButton>(
  options: ButtonOptions = defaultButtonOptions
): ElementViewTemplate<T> {
  return html<T>`
    <button
      class="control"
      part="${options.shadowPartNames.button}"
    >
    </button>
  `;
}

This way the current default naming scheme would still exist, but also allow consumers of the Foundation templates to override with unique part names when creating the component definitions. So now if we override the part names in TextField and Button through their template options our template could look like:

const myElTemplate = html`
  <my-text-field exportparts="btn-control"></my-text-field>
  <my-button exportparts="txt-control"></my-button>
`;

Now we can target specific nested parts for external styling my-el::part(btn-control) and my-el::part(txt-control)

@KingOfTac KingOfTac added the status:triage New Issue - needs triage label Nov 17, 2022
@chrisdholt
Copy link
Member

I’m out this week and taking a break from most things here as well, but wanted to provide a quick response. I think this is “planned” with the Foundation work we’ve discussed and continue to move towards as you’ve noted. On my phone but @EisenbergEffect might know off the top of my head which stage of changes we’ll look at this. With that said, I’m not going to go much into the details on implementation now, but this is certainly something we can consider as part of this.

What I will say is that I think changing foundation to enable this by default would be a bad design. As you’ve noted, it requires breaking encapsulation and moving towards what I believe is a redundant naming convention (button-control vs just “control”) in order to manage composition with a prescriptive approach. While I get the desire, I think that an alternative here is forking and managing templates yourself in the interim. While less than desirable, I think that would be more reasonable than hoisting the requirement or assumption that someone both wants to expose parts and needs to deeply nest, etc.

@KingOfTac if the foundation issue is still open which describes this, can we move this and consolidate as part of that issue perhaps? As it stands here it appears like a gap which is unplanned rather than discussion/suggestion for that stage. If it’s closed, let’s reference the issue here perhaps?

@EisenbergEffect
Copy link
Contributor

EisenbergEffect commented Nov 17, 2022

I believe the template/anatomy builder proposed in #5901 (iteration 5) will enable this type of renaming as needed by design system authors.

@KingOfTac
Copy link
Collaborator Author

I’m out this week and taking a break from most things here as well, but wanted to provide a quick response. I think this is “planned” with the Foundation work we’ve discussed and continue to move towards as you’ve noted. On my phone but @EisenbergEffect might know off the top of my head which stage of changes we’ll look at this. With that said, I’m not going to go much into the details on implementation now, but this is certainly something we can consider as part of this.

What I will say is that I think changing foundation to enable this by default would be a bad design. As you’ve noted, it requires breaking encapsulation and moving towards what I believe is a redundant naming convention (button-control vs just “control”) in order to manage composition with a prescriptive approach. While I get the desire, I think that an alternative here is forking and managing templates yourself in the interim. While less than desirable, I think that would be more reasonable than hoisting the requirement or assumption that someone both wants to expose parts and needs to deeply nest, etc.

@KingOfTac if the foundation issue is still open which describes this, can we move this and consolidate as part of that issue perhaps? As it stands here it appears like a gap which is unplanned rather than discussion/suggestion for that stage. If it’s closed, let’s reference the issue here perhaps?

Pretty much the response I was expecting, but wasn't sure what had been discussed on the subject so far, so I thought I'd bring it up through RFC, with people being out right now this seemed like the best way to bring it up without getting lost.

@KingOfTac KingOfTac reopened this Nov 17, 2022
@KingOfTac
Copy link
Collaborator Author

Moving this over to #5901 to consolidate features for the builder there.

@KingOfTac KingOfTac closed this as not planned Won't fix, can't repro, duplicate, stale Nov 17, 2022
@chrisdholt
Copy link
Member

Thanks @KingOfTac!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:triage New Issue - needs triage
Projects
None yet
Development

No branches or pull requests

3 participants