Foundation: update templates for structure and consistency #6286

bheston · 2022-08-16T23:15:50Z

Pull Request

📖 Description

As part of the next major release of fast-foundation, we wanted to do a complete review of the component templates and align the structure and naming as much as possible.

This PR unifies the naming and cleans up some test styles, especially around the recent update to the start and end slots.

Before on the left, update on the right. "C" = class, "P" = part, "R" = role, "S" = slot. 💔 = potentially style-breaking.

Simple containers

Accordion (migrated to #6796)

Remove unused <template> wrapper

Complex containers

Breadcrumb (migrated to #6797)

Add "start" and "end" slots

Dialog

💔 Flatten "positioning-region" element to host

Disclosure

Update to use "start" and "end" helpers
💔 Rename “disclosure” to “details”
💔 Rename “invoker” to “summary”
💔 Rename “disclosure-content” to “content”
Add part to "details", "summary", and "content"

Radio group

💔 Rename “positioning-region” to “control”
💔 Remove “horizontal” or “vertical” class, use attribute on host instead

Toolbar

💔 Rename “positioning-region” to “control”

Inputs

Number field (migrated to #6798)

💔 Rename “control” to “field”
💔 Rename “root” to “control”
💔 Rename “controls” to “step-buttons”
💔 Rename “step-up-glyph” to “step-up-icon”
💔 Rename “step-down-glyph” to “step-down-icon”

Search (migrated to #6798)

💔 Rename “control” to “field”
💔 Rename “root” to “control”
💔 Remove “input-wrapper”
💔 Rename “clear-glyph” to “clear-icon”

Switch (migrated to #6798)

💔 Rename “switch” to “control”
💔 Rename "switch" slot to "thumb"
Add default "thumb" with class and part

Text area (migrated to #6798)

💔 Rename “control” to “field”
Add wrapper element “control”
Add "start" and "end" slots

Text field (migrated to #6798)

💔 Rename “control” to “field”
💔 Rename “root” to “control”

Start, content, end pattern

Badge (migrated to #6797)

Add "start" and "end" slots
💔 Rename “control” to “content”

Breadcrumb item (migrated to #6797)

💔 Flatten “listitem” div to template

Tab

Wrap default slot in “content”

Items with children

Accordion item (migrated to #6796)

💔 Rename “icon” to “expand-collapse-icon”
💔 Move “start”, "end", and "expand-collapse-icon" inside “button”
💔 Rename "region" class and part to "panel"

Combobox

💔 Rename “indicator” to “open-close-icon” (slot and wrapper)
💔 Move “open-close-icon” outside of “control” slot
💔 Move “end” slot before “open-close-icon”
💔 Rename "selected-value" to "field"
💔 Rename "control" slot to "field"

Horizontal scroll (migrated to #6921)

💔 Rename "content-container" to "content"
💔 Rename "scroll-prev" to "scroll-previous"
💔 Remove "scroll-action" containers
Add default flipper content with parts

Menu item

💔 Remove “input-container”
💔 Rename “checkbox” to “checkbox-indicator”
💔 Rename “radio” to “radio-indicator”
💔 Remove “expand-collapse-glyph-container”
💔 Rename “expand-collapse” to “submenu-icon”
💔 Rename “expand-collapse-indicator” to “submenu-icon”

Select

💔 Align to ARIA spec
💔 Rename “indicator” to “open-close-icon” (slot and wrapper)
💔 Remove “control” container
💔 Move “open-close-icon” outside of “control” container
💔 Add "button" element inside "button-container"
💔 Move "start" and “end” slots into "button"
Add "listbox-container" slot

Tree item

💔 Remove “positioning-region” element
💔 Rename “content-region” to “control”
💔 Rename “expand-collapse-glyph” to “expand-collapse-icon”
Add “content” wrapper around default slot

Everything else

Avatar

💔 Remove "backplate" container
Add "content" wrapper around default slot

Progress (migrated to #6799)

💔 Rename "progress" container to "determinate" and "indeterminate"
💔 Change “determinate” div to slot
💔 Remove “indeterminateIndicator2” option

Progress ring (migrated to #6799)

💔 Follow Progress structure

Slider (migrated to #6800)

💔 Rename “track-container” part to “track”
Add default "thumb" with class and part
💔 Remove duplicate class (orientation) on root template element

Slider label (migrated to #6800)

💔 Flatten “container” and “root”
💔 Rename “label” to “content”
Add part to "mark" and "content"

👩‍💻 Reviewer Notes

Review the Storybook site, or the illustrations of the components before and after, compared to the changes in code.

📑 Test Plan

Tested via comparison against proposal and in Storybook.

✅ Checklist

General

I have included a change request file using $ yarn change
I have added tests for my changes.
I have tested my changes.
I have updated the project documentation to reflect my changes.
I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

I have added a new component
I have modified an existing component

EisenbergEffect · 2022-08-16T23:21:21Z

I don't have strong opinions on how these should be structured. That's more the area of @chrisdholt. I'll just review the code itself independent of those decisions.

packages/web-components/fast-foundation/src/accordion-item/accordion-item.template.ts

packages/web-components/fast-foundation/src/badge/badge.template.ts

packages/web-components/fast-foundation/src/breadcrumb-item/breadcrumb-item.template.ts

packages/web-components/fast-foundation/src/combobox/combobox.ts

packages/web-components/fast-foundation/src/combobox/combobox.template.ts

chrisdholt · 2022-08-16T23:38:04Z

packages/web-components/fast-foundation/src/dialog/dialog.template.ts

@@ -7,31 +7,29 @@ import type { FASTDialog } from "./dialog.js";
 */
 export function dialogTemplate(): ElementViewTemplate<FASTDialog> {
    return html<FASTDialog>`
-        <div class="positioning-region" part="positioning-region">


This is one I'll need to pull down and play with to put it through it's paces, but moving that to the host here should work for all considerations I can think of.

It worked for the ones I considered as well. There's a note in the spec about why it was structured that way, but I couldn't find a way it didn't work like this.

packages/web-components/fast-foundation/src/disclosure/disclosure.template.ts

chrisdholt · 2022-08-16T23:50:06Z

packages/web-components/fast-foundation/src/menu-item/menu-item.template.ts

-                    <div part="input-container" class="input-container">
-                        <span part="checkbox" class="checkbox">
-                            <slot name="checkbox-indicator">
-                                ${options.checkboxIndicator || ""}
-                            </slot>
-                        </span>
-                    </div>
+                    <span part="checkbox-indicator" class="checkbox-indicator">
+                        <slot name="checkbox-indicator">
+                            ${options.checkboxIndicator || checkboxIndicator}
+                        </slot>
+                    </span>
                `
            )}
            ${when(
                x => x.role === MenuItemRole.menuitemradio,
                html<FASTMenuItem>`
-                    <div part="input-container" class="input-container">
-                        <span part="radio" class="radio">
-                            <slot name="radio-indicator">
-                                ${options.radioIndicator || ""}
-                            </slot>
-                        </span>
-                    </div>
+                    <span part="radio-indicator" class="radio-indicator">
+                        <slot name="radio-indicator">
+                            ${options.radioIndicator || radioIndicator}
+                        </slot>
+                    </span>


These are both typically in the same spot because an item is either a menuitem, menuitemcheckbox, or menuitemradio, so I'm wondering if we can simplify this with something that serves a single indicator but sets the class and part based on its role. So a single conditional check with the part/class set based on the role. Thoughts? I think it simplifies the template and still accomplishes the same thing.

Actually, I think we may only need one of these and I think it should likely still be called checked-indicator.

While I know that the initial fast-components made these look like a checkbox and an radio, the actual indicator represents the checked state of either an element with one of the checkable roles. I'd like to dig in here, because I think this can simplify and while someone may choose to make it look like one or the other, that's also primarily state based that can be handled in styles or using a when directive from the view to manage sending alternative svg's here. In other implementations, it may be the same SVG, but the behavior only allows single selection.

See my comment on progress for one potential way to slim down this block by removing the slot. I don't see a case for individual components to slot in a custom indicator here.

I do think it's beneficial to be able to provide the icons individually, the same icon if that's what your design called for, but I think it's good UX to have different icons. I'd hate to see the css get messier to accommodate a minor reduction.

Again, I don't think that is prohibited by having a singular slot. These could be handled in configuration using a when directive without needing to hoist that requirement to all implementations. If the goal is whether we can accomplish what you've stated above I think we should use the most concise method of addressing that. I commented below but the removal of slots in general is a non-starter. The options don't replace it, they provide a method of configuring a global default. That helps for "menu" but if I need to configure something else there in a one-off scenario, the slot is still massively more useful.

A couple of thoughts here.

Let's decouple the icons as mentioned above.

Should we make raio/checkbox support an option and only compose those in if present? We could make this really simple. See the label options sampele above.

If we keep radio/checkbox in by default, I think we should remove the use of the double when and instead just render a property that returns a template based on a single condition check. That will be slightly better in terms of performance.

Yes ^ - My big question here is whether the better ergonomic is baking the conditional rendering in or letting that be handled by the person implementing. On one hand, in the case I don't need two I need to pass the same thing twice. Alternatively, if I have two different SVG's I can handle my own when, right? Does one match our composition value more than the other?

I'd love to find a way to slim down the template here if that's what @EisenbergEffect's last comment suggests. I'm not sure I am seeing the exact change though.

From a component framework perspective, the person authoring the component doesn't know if someone is going to use checkbox or radio or not. Since the component supports this to fully support the ARIA spec it should also have a clear way to have two different indicators. This should be easy without having to worry about it for each menu you implement.

I've consolidated this to a single conditional using the pattern from Flipper.

I understand that someone may not always need checked functionality, and the only feasible solution I see for that is using the template builder to exclude it. For now we have that role capability built in.

From a visual perspective, it's bad usability for two things that operate differently to have the same look, so I can't in good conscience agree with a solution where the easy default is to treat them the same. The fact that they both relate to the checked state is not the sole decision or the ARIA spec could have omitted the two roles and encouraged people to implement radio functionality in code.

This is at least better than what we had, and if we want to consider changing this, though my vote is not to, we should pull it into a new issue.

packages/web-components/fast-foundation/src/menu-item/menu-item.template.ts

chrisdholt · 2022-08-16T23:53:20Z

packages/web-components/fast-foundation/src/menu-item/menu-item.template.ts

        ${when(
            x => x.hasSubmenu,
            html<FASTMenuItem>`
-                <div
-                    part="expand-collapse-glyph-container"
-                    class="expand-collapse-glyph-container"
-                >
-                    <span part="expand-collapse" class="expand-collapse">
-                        <slot name="expand-collapse-indicator">
-                            ${options.expandCollapseGlyph || ""}
-                        </slot>
-                    </span>
-                </div>
+                <span part="submenu-icon" class="submenu-icon">
+                    <slot name="submenu-icon">
+                        ${options.submenuIcon || submenuIcon}
+                    </slot>
+                </span>


@EisenbergEffect do you think this and the below code snippet might be a good candidate for one of the new conditional directives?

Note that the expanded check changes with the menu updates i have in the RFC for removing anchored region...

It's actually simpler than that I think. We don't need render to do this. Instead, we can have a single computed property that based on the conditions returns just one template. That will be cheaper than multiple when instances.

packages/web-components/fast-foundation/src/progress/progress.template.ts

bheston · 2022-08-17T18:55:12Z

Related to #5806

packages/web-components/fast-foundation/.storybook/preview-body.html

packages/web-components/fast-foundation/src/accordion-item/accordion-item.template.ts

packages/web-components/fast-foundation/src/progress-ring/progress-ring.template.ts

chrisdholt · 2022-08-17T20:30:22Z

packages/web-components/fast-foundation/src/radio-group/radio-group.template.ts

-                    x.orientation === Orientation.horizontal ? "horizontal" : "vertical"}"
-                part="positioning-region"
-            >
+            <div class="radiogroup" part="radiogroup">


Larger question here is whether it's worth keeping the attribute for horizontal and vertical. If we decide to keep it there is an argument for keeping a class here out of convenience. If we opt for removal then the primary method of adding would require extending the class (which I don't think is the worst thing). Given that there isn't a functional purpose, I'd be for removing the attribute from here as vertical and horizontal is primarily a visual consideration which would track similar to appearances on other controls.

If we remove this, should we have the CLI set up the default by adding this back in? I saw a customer just the other day indicating that this was important. So, if we remove it from the base, I'm thinking it might be nice to add it via the CLI.

Yes - And on this, the confusion I believe is that the attribute exists on the Foundation component but then no styling is delivered. I'd propose that vertical in this case is largely a design consideration. There's no functional requirement for it and no impact to AT currently by it existing. One could add an attribute for it by extending the component (what we should do in the CLI), perhaps they want to use data- instead to just apply the visual style. I think the attribute can be removed from Foundation but it's a good example on how to extend and a pattern we value, so a good fit for the CLI implementation. Open here, but I do think that it's confusing to have this in Foundation when it only serves a visual purpose. That follows along with not baking semantics on appearances into foundation.

The aria-orientation attribute is an inherited property for the radiogroup role. A note for the attribute indicates that ARIA 1.1 changed the default value from being horizontal to undefined: https://w3c.github.io/aria/#radiogroup:~:text=In%20ARIA%201.1%2C%20the%20default,ambiguous%20(e.g.%2C%20radiogroup).

It can still be passed to the host where the role is, we just don't need to actually use it within the class or template, so I'm not sure it needs to be enumerated.

The goal in removing it was bringing the selectors closer together. There have been some specificity issues, though this may not be one of them. Since there was no functional changes based on the orientation, using the attribute in the selector seemed like a good way to simplify the overall component. I suppose that could be said for all class selectors, though we've decided they are worth the overhead for internal styling convenience. By that logic, perhaps it's best to leave this here.

packages/web-components/fast-foundation/src/search/search.ts

packages/web-components/fast-foundation/src/select/select.template.ts

packages/web-components/fast-foundation/src/slider-label/slider-label.template.ts

packages/web-components/fast-foundation/src/slider/slider.template.ts

packages/web-components/fast-foundation/src/tab/stories/tab.register.ts

packages/web-components/fast-foundation/src/text-area/text-area.template.ts

packages/web-components/fast-foundation/src/text-field/text-field.template.ts

github-actions · 2023-01-31T00:05:06Z

Azure Static Web Apps: Your stage site is ready! Visit it here: https://purple-ocean-0b7ce3410-6286.centralus.azurestaticapps.net

github-actions · 2023-01-31T01:21:45Z

Azure Static Web Apps: Your stage site is ready! Visit it here: https://purple-ocean-0b7ce3410-6286.centralus.azurestaticapps.net

chrisdholt · 2023-02-11T00:17:12Z

packages/web-components/fast-foundation/src/accordion-item/accordion-item.template.ts

+                ${startSlotTemplate(options)}
                <span class="heading-content" part="heading-content">
                    <slot name="heading"></slot>
                </span>
+                ${endSlotTemplate(options)}
+                <span class="expand-collapse-icon" part="expand-collapse-icon" aria-hidden="true">
+                    <slot name="expand-collapse-icon">
+                        ${staticallyCompose(options.expandCollapseIcon)}
+                    </slot>
+                <span>


Revisiting this and I don't think we want these within the button itself because it will group all content as part of the label. While I can definitely see a case for the expand-collapse-icon, moving the others in could actually generate some new a11y issues for folks under the assumption that the only slotted content here is icons (which I don't think would be an accurate assumption).

This might be a case of an ambiguous definition in the ARIA spec. At https://w3c.github.io/aria-practices/#accordion, under "Roles, States, and Properties" bullet 2.2 says "The button element is the only element inside the heading element." Agreed that if slotted content is something else like a Menu Button with interactions for the section it might confuse what is announced as the title. I believe this is true for the start and end slot placement in other components as well, though maybe less likely that we expect content other than icons. Do we have any general guidance re: a11y on slotted content?

I tested this on Anchor, for instance with a Badge, and it reads the badge content the same in both cases. In some cases that may make sense, like announcing something as "New". If they are Buttons instead, focus goes to the accordion button, then tab to start, end, then the next accordion item, which seems like the flow I expect.

Note that this change (at least currently) is only the sequence of items, not the hierarchy. This should not have any a11y impact that I know of, it only seeks to align the named intent of the "start" and "end" slots with their document order to minimize the need to resequence with css.

I think the intent with not including these as part of the button was that there may be scenarios where people may place other interactive elements within these slots and placing those inside the button would create a problem there. I think this is a hierarchy change because these are moving to be nested within the button, which means anything projected into the slot is a child of the button now instead.

Uh, looks like I didn't look at the diff correctly. Of course those elements moved.

I looked back at the spec, and I don't see a way to make both of the following statements true given the additional elements we have. I agree that the previous hierarchy (I've shifted the sequence) is best.

The button element is the only element inside the heading element.
That is, if there are other visually persistent elements, they are not included inside the heading element.

chrisdholt · 2023-02-11T00:24:54Z

packages/web-components/fast-foundation/src/combobox/combobox.template.ts

                </slot>
                ${endSlotTemplate(options)}
+                <div class="open-close-icon" part="open-close-icon" aria-hidden="true">


this is a nit but I'd suggest a span here as the element is intended to be inline.

@bheston I need to pull down and test a couple things here on the a11y end of things. I think it's fine but need to validate the tree with certain slotted items.

packages/web-components/fast-foundation/src/horizontal-scroll/horizontal-scroll.template.ts

github-actions · 2023-02-14T20:11:40Z

Azure Static Web Apps: Your stage site is ready! Visit it here: https://purple-ocean-0b7ce3410-6286.centralus.azurestaticapps.net

github-actions · 2023-04-25T23:55:09Z

Azure Static Web Apps: Your stage site is ready! Visit it here: https://purple-ocean-0b7ce3410-6286.centralus.azurestaticapps.net

Dismissing my review to unblock this Pr

janechu · 2024-06-14T16:40:19Z

Closing this due to the sheer number of conflicts and this also seems like it might break downstream CSS for consumers.

bheston requested review from EisenbergEffect, chrisdholt, nicholasrice and janechu as code owners August 16, 2022 23:15

chrisdholt reviewed Aug 16, 2022

View reviewed changes

packages/web-components/fast-foundation/src/accordion-item/accordion-item.template.ts Outdated Show resolved Hide resolved

chrisdholt reviewed Aug 16, 2022

View reviewed changes

packages/web-components/fast-foundation/src/accordion-item/accordion-item.template.ts Outdated Show resolved Hide resolved

chrisdholt reviewed Aug 16, 2022

View reviewed changes

packages/web-components/fast-foundation/src/badge/badge.template.ts Outdated Show resolved Hide resolved

chrisdholt reviewed Aug 16, 2022

View reviewed changes

packages/web-components/fast-foundation/src/breadcrumb-item/breadcrumb-item.template.ts Show resolved Hide resolved

chrisdholt reviewed Aug 16, 2022

View reviewed changes

packages/web-components/fast-foundation/src/combobox/combobox.ts Outdated Show resolved Hide resolved

chrisdholt reviewed Aug 16, 2022

View reviewed changes

packages/web-components/fast-foundation/src/combobox/combobox.template.ts Outdated Show resolved Hide resolved

chrisdholt reviewed Aug 16, 2022

View reviewed changes

packages/web-components/fast-foundation/src/disclosure/disclosure.template.ts Outdated Show resolved Hide resolved

chrisdholt reviewed Aug 16, 2022

View reviewed changes

packages/web-components/fast-foundation/src/menu-item/menu-item.template.ts Outdated Show resolved Hide resolved

chrisdholt reviewed Aug 16, 2022

View reviewed changes

bheston commented Aug 17, 2022

View reviewed changes

packages/web-components/fast-foundation/src/progress/progress.template.ts Show resolved Hide resolved

radium-v reviewed Aug 17, 2022

View reviewed changes

packages/web-components/fast-foundation/.storybook/preview-body.html Outdated Show resolved Hide resolved

radium-v reviewed Aug 17, 2022

View reviewed changes

packages/web-components/fast-foundation/src/accordion-item/accordion-item.template.ts Outdated Show resolved Hide resolved