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: rework fast-foundation templates to be leaner and support focused scenarios #5806

Closed
chrisdholt opened this issue Apr 6, 2022 · 13 comments
Labels
area:fast-foundation Pertains to fast-foundation breaking-change A breaking change to a shipping package closed:obsolete No longer valid community:noteworthy An issue or PR of particular interest to the community or planned for an announcement. improvement A non-feature-adding improvement

Comments

@chrisdholt
Copy link
Member

🔦 Context

The initial focus of the fast-foundation package was to support various implementations. Rather than taking a focused approach to support common scenarios, we approached the problem by trying to support a majority of scenarios which meant adding DOM, additional hooks, etc. Over time, it's become clear that this has made it somewhat difficult for teams to adopt only what is needed. This means that in a majority of scenarios, users may be taking a dependency on more than they need.

This RFC proposes a better way, which is more aligned with our principles and approach. The primary outcome from this change will be removing unnecessary DOM nodes which will lead to easier to implement templates, increased performance with less DOM, etc.

In the event that the templates don't work for a given approach, folks are free to fork and leverage customized templates to accomplish their own scenarios. In the event that a template doesn't work for the fast-components, we should - as with everything else, live under the same rules as the community and fork the template if it doesn't support a specific scenario. Based on my research that seems unlikely, but I think it's important to be clear that the foundation package exists for the community and not explicitly our implementation.

💻 Examples

A simple example of this would be seen with primitives that leverage the start and end slots. Initially, added DOM and methods were included to support significant deltas in styling. Over time it's become apparent that this often isn't needed by many folks and the existing FAST (and Fluent UI) components can easily be styled without them.

Taking an example of button:

Current final DOM

<template>
    <span part="start">
         <slot name="start"></slot>
    </span>
    <span part="content">
            <slot></slot>
    </span>
    <span part="end">
         <slot name="end"></slot>
    </span>
</template>

Proposed DOM

```html
<template>
    <slot name="start"></slot>
    <slot></slot>
    <slot name="end"></slot>
</template>
@chrisdholt chrisdholt added improvement A non-feature-adding improvement status:triage New Issue - needs triage area:fast-foundation Pertains to fast-foundation area:fast-components community:noteworthy An issue or PR of particular interest to the community or planned for an announcement. breaking-change A breaking change to a shipping package labels Apr 6, 2022
@chrisdholt chrisdholt pinned this issue Apr 6, 2022
@EisenbergEffect
Copy link
Contributor

I agree with this type of change. One thing I'd want to pair with this is that we beef up our documentation so that it has a proper page or two on how to customize components with some real examples.

@EisenbergEffect EisenbergEffect removed the status:triage New Issue - needs triage label Apr 6, 2022
@rajsite
Copy link

rajsite commented Apr 6, 2022

In the event that the templates don't work for a given approach, folks are free to fork and leverage customized templates to accomplish their own scenarios.

Our biggest concern with forking templates is maintenance if additions are added or changes happen to templates upstream. How can we make sure we have merged in relevant changes when upgrading fast versions?

For most of the customizations we have done we have wanted to insert additional slots for some visual flourish (an extra notification image slot on tabs, an error text line below the text-field) so forking the entire template (which has things like event binding, etc) is usually overkill.

If there was an api to help insert template bits into exisiting templates:

class MyTextField extends FoundationTextField {}

html<MyTextField>`
<!-- Foundation Text Field Template -->
<template>
    <slot name="start"></slot>
    <slot></slot>
    <slot name="end"></slot>
</template>
`.insertAdjacentTemplate('slot[name="end"]', 'afterend', html<MyTextField>`
    <!-- MyTextField template insertions -->
    <slot name="errortext"></slot>
`);

That would help with a common template change we have made.

However for larger forks I could see it being generally useful to have a pattern to know when changes are made upstream that we need to be aware of.

Maybe component libraries should be expected to have a test for each forked template that compares a hash of the contents from when the template was forked. So on upgrades a change in hash will fail the test and require updating the hash in test or something.

@scomea
Copy link
Collaborator

scomea commented Apr 6, 2022

We frequently touch templates when fixing bugs, we should be careful about encouraging custom templates, esp. for more complex components.

@chrisdholt
Copy link
Member Author

We frequently touch templates when fixing bugs, we should be careful about encouraging custom templates, esp. for more complex components.

Unfortunately I just don't think this scales. The reality is that someone may find value from the class only. I don't think it's our responsibility to police how people compose things or where they decide they want to borrow, leverage, eject, etc. Trying to solve all the problems for all scenarios doesn't really scale well into production scenarios. The cost of hoisting requirements to all users that aren't universal create additional tension, especially when the alternative at that point is the same - forking. Again, this comes down in my mind to goals and scale - supporting all the things ultimately ends up hurting our goals and costing us in the long run. The issue of us touching templates I feel is a symptom of us not playing fair with Foundation. We're touching those to solve issues we created with our implementation, and other teams often pay the price for that. This proposes a solution where that is no more. We all play by the same rules and rails. When we need something to work outside of that, we have a clean and easy way to accomplish that. Foundation isn't for us as maintainers, there are several teams building their design systems off it - they shouldn't pay the price for requirements which are not explicitly shared by the broader community. There is and needs to remain a clear separation of concerns between Foundation (baseline primitives) and Components (implementation).

Our biggest concern with forking templates is maintenance if additions are added or changes happen to templates upstream. How can we make sure we have merged in relevant changes when upgrading fast versions?

I think this is a fair concern, but I'll again mention that I think some of the deltas here have come from us tightly coupling foundation with our implementation. For button, there is logic that is tightly coupled to how we style icon buttons in the Foundation template - I think that's wrong. In a scenario where we need that hook in Foundation, we need to extend both the class and the template. This was done with appearances, but it should really be done for anything that doesn't represent a foundation button. In this scenario, I think template updates would be rare because if a change needs to be made for the sake of components, we (FAST) need to be mindful of that change.

For most of the customizations we have done we have wanted to insert additional slots for some visual flourish (an extra notification image slot on tabs, an error text line below the text-field) so forking the entire template (which has things like event binding, etc) is usually overkill.

If there was an api to help insert template bits into exisiting templates...

Adding @EisenbergEffect for this - we were looking at potential new API's for this. As far as how to keep things aligned, it would be interesting to hear proposals from the community on tooling - but I don't think it should block this improvement. It can be done async or once we have a path forward. :)

@yinonov
Copy link
Contributor

yinonov commented Apr 7, 2022

I think the following relates to the pain point rajsite raises.

another option I'd like to suggest here (and I think is a low hanging fruit) is -
extracting ARIAGlobalStatesAndProperties template members to a pattern that can be included with a single line.
when overriding a fast-foundation element template we must copy & paste and track changes of

aria-atomic="${x => x.ariaAtomic}"
aria-busy="${x => x.ariaBusy}"
aria-controls="${x => x.ariaControls}"
aria-current="${x => x.ariaCurrent}"
aria-describedby="${x => x.ariaDescribedby}"
aria-details="${x => x.ariaDetails}"
aria-disabled="${x => x.ariaDisabled}"
aria-errormessage="${x => x.ariaErrormessage}"
aria-expanded="${x => x.ariaExpanded}"
aria-flowto="${x => x.ariaFlowto}"
aria-haspopup="${x => x.ariaHaspopup}"
aria-hidden="${x => x.ariaHidden}"
aria-invalid="${x => x.ariaInvalid}"
aria-keyshortcuts="${x => x.ariaKeyshortcuts}"
aria-label="${x => x.ariaLabel}"
aria-labelledby="${x => x.ariaLabelledby}"
aria-live="${x => x.ariaLive}"
aria-owns="${x => x.ariaOwns}"
aria-pressed="${x => x.ariaPressed}"
aria-relevant="${x => x.ariaRelevant}"
aria-roledescription="${x => x.ariaRoledescription}"

if the focus is also making templates flexible then an aspiration is to just include a pattern / portion.
something like

ViewTemplate<Button> = (context: ElementDefinitionContext) => html`
  <button
    ${() => spread(ARIAGlobalStatesAndProperties)}
  >
    <!-- whatever content -->
  </button>
`;

I was about to mention open-wc' lit helpers methods but I notice now they're removed so not sure what caveats this implementation holds
open-wc/open-wc@f3cbb2a

extending foundation base classes is a close alternative to extending customized built-in element. perhaps the focus should always be ground zero for atomic elements.

@EisenbergEffect
Copy link
Contributor

@chrisdholt The kind of spread operation that @yinonov mentions is not possible in FE1 today. However, if we aligned this set of changes to FE2, the new directive work I was talking about the other day would make it possible to do spreads since directive will be able to add multiple behaviors with that change.

@EisenbergEffect
Copy link
Contributor

EisenbergEffect commented Apr 7, 2022

@yinonov In the specific case of aria attributes, we've been working on a nicer, more performant solution than what you show above. So, we've also got specific new stuff for that case in addition to the general problem you bring up with developer experience. (We did a lot of performance testing recently and found that our approach to aria attributes could be greatly improved.)

@yinonov
Copy link
Contributor

yinonov commented Apr 7, 2022

great news then @EisenbergEffect. looking forward to see these updates

@simonxabris
Copy link
Contributor

In our design system we rely heavily on the mechanism that for example when the start slot has assigned nodes, the wrapping span has a .start class and when there is no assigned nodes, there is no .start class. If this were to go away the update path for us would be pretty steep. I don't know if it's representative to how the community uses Foundation, but maybe it's something to keep in mind.

I'm in complete agreement with @rajsite's point. We actually never ever forked a Foundation element's template, because maintaining it with the upstream changes seemed like a lot of work. Whenever we needed changes we just appended some elements to the original template, so an API to make this easier and more transparent that we're modifying the original template would be a welcome addition.

@yinonov
Copy link
Contributor

yinonov commented Apr 13, 2022

here's a concern worth mentioning, setting parts on shadowRoot elements makes them exposed for authors to customize easily. when locking down components to enforce a design system rules, setting parts can conflict with the general intention.

one can obviously query all part attributes from within the class and remove 'em programmatically.

still, not sure what the tradeoffs are. wondering what other design system maintainers feel about that...

@chrisdholt
Copy link
Member Author

here's a concern worth mentioning, setting parts on shadowRoot elements makes them exposed for authors to customize easily. when locking down components to enforce a design system rules, setting parts can conflict with the general intention. one can obviously querying all part attributes from within the class and remove 'em programmatically.

still, not sure what the tradeoffs are. wondering what other design system maintainers feel about that...

This one is definitely interesting; I’d be open to a separate issue raising this as a potential enhancement (templates with and without) to gather feedback and consider how/if this could be best addressed. Thoughts on opening an issue specifically for that @yinonov?

@yinonov
Copy link
Contributor

yinonov commented Apr 13, 2022

of course 👌 #5832

@janechu
Copy link
Collaborator

janechu commented May 29, 2024

Unfortunately @microsoft/fast-components has been deprecated for some time.

@janechu janechu closed this as completed May 29, 2024
@janechu janechu added the closed:obsolete No longer valid label May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:fast-foundation Pertains to fast-foundation breaking-change A breaking change to a shipping package closed:obsolete No longer valid community:noteworthy An issue or PR of particular interest to the community or planned for an announcement. improvement A non-feature-adding improvement
Projects
None yet
Development

No branches or pull requests

7 participants