[OptionList] consolidate se23 styles and logic #10177

gwyneplaine · 2023-08-23T00:11:40Z

WHY are these changes introduced?

Fixes #9953
Fixes #10197

WHAT is this pull request doing?

OptionList consolidate se23 logic
OptionList consolidate se23 styles
Remove custom Checkbox component in favour of the standard Polaris Checkbox
Removes the optionRole prop from the OptionList component. This is presently being used to toggle the presentation role on the custom checkbox input. This PR removes this prop for a few reasons:
- Its no longer used, since the custom Checkbox has been deleted.
- The functionality it was providing (toggling presentation role) doesn't seem to be supported by browsers. The browser ignores the presentation role on all focusable elements (including inputs). See excerpt from MDN below
- A cursory Grokt search, rules out internal usage of this prop

How to 🎩

Storybook
Prod storybook

🎩 checklist

Tested on mobile
Tested on multiple browsers
Tested for accessibility
Updated the component's README.md with documentation changes
Tophatted documentation changes in the style guide

gwyneplaine · 2023-08-23T00:22:22Z

polaris-react/src/components/OptionList/components/Option/Option.tsx

  );

-  const checkBoxRole = role === 'option' ? 'presentation' : undefined;


According to the MDN the presentation role is ignored on all focusable elements (including inputs), so this would just default back to the implicit 'checkbox' role in the browser.

After a cursory screen reader test, I also couldn't find any behavioural differences from this change.

gwyneplaine · 2023-08-23T00:23:30Z

polaris-react/src/components/OptionList/components/Option/Option.tsx

-            onChange={handleClick}
-          />
-        ) : (
-          <Checkbox


Since we're no longer using the custom checkbox (or exporting it from root), its also deleted in this PR.

gwyneplaine · 2023-08-23T00:25:22Z

polaris-react/src/components/OptionList/OptionList.tsx

@@ -28,8 +26,6 @@ export interface OptionListProps {
  options?: OptionDescriptor[];
  /** Defines a specific role attribute for the list itself */
  role?: 'listbox' | 'combobox' | string;
-  /** Defines a specific role attribute for each option in the list */
-  optionRole?: string;


I think removing the prop given the reasons outlined in the PR description is the right thing to do, however I'll defer to the team here as to whether or not we want to deprecate first.

I think your grokt search makes this pretty safe to remove, @kyledurand is there a list or somewhere where we are tracking the changed props for v12?

Given the context you provided, if the optionRole prop was only used in Option and with the custom checkbox component that was rendered without the beta flag, I think it makes sense to remove it with the v12 release 👍

@gwyneplaine "Component changes" tasklist in this issue can be updated to included optionRole prop removal

gwyneplaine · 2023-08-23T04:43:45Z

I'm incredibly stumped as to why this UI test changed. Can't find a specific change in the PR that would lead to the change.
Free bagel to whoever can help me figure it out 😢

laurkim · 2023-08-23T16:06:02Z

I'm incredibly stumped as to why this UI test changed. Can't find a specific change in the PR that would lead to the change. Free bagel to whoever can help me figure it out 😢

I wonder if this is coming from the fact that the Bleed component no longer wraps the Box ul? Visually there's no difference but the chromatic baseline on next might be creating a new baseline due to the changes in the rendered elements.

laurkim

Nice 🙌 🌟

sophschneider

Nice clean up!! I think the UI baseline is good to approve it might just be a dom change but visually it looks the same to me! idk if that answer warrants a bagel or not 🥯😅

sophschneider · 2023-08-23T15:49:24Z

polaris-react/src/components/OptionList/OptionList.tsx

@@ -28,8 +26,6 @@ export interface OptionListProps {
  options?: OptionDescriptor[];
  /** Defines a specific role attribute for the list itself */
  role?: 'listbox' | 'combobox' | string;
-  /** Defines a specific role attribute for each option in the list */
-  optionRole?: string;


I think your grokt search makes this pretty safe to remove, @kyledurand is there a list or somewhere where we are tracking the changed props for v12?

polaris-react/src/components/OptionList/OptionList.tsx

polaris-react/src/components/OptionList/components/Option/Option.scss

sophschneider · 2023-08-23T16:19:44Z

polaris.shopify.com/content/components/lists/option-list.md

@@ -76,4 +76,3 @@ Controls in simple option lists are [buttons](https://polaris.shopify.com/compon
 If you customize the option list, you can provide ARIA roles that fit the context. These roles must be valid according to the [W3C ARIA specification](https://www.w3.org/TR/wai-aria-1.1/) to be conveyed correctly to screen reader users.

 - The `role` prop adds an ARIA role to the option list wrapper
- The `optionRole` prop adds an ARIA role to the option list items


@yurm04 would this conflict at all with your polaris.shopify.com changes?

### WHY are these changes introduced? Resolves #10035. Updates existing `tall-chickens-repeat` changeset with all the component consolidation work (included any in progress/ready for review/up next/backlog PRs as well). Adds changesets for [OptionList](#10177) `optionRole` and [Page](#10187) `divider` prop removal.

* next: (87 commits) [ButtonGroup] Removed `segmented` boolean prop and replaced with `variant` (#9997) [LegacyTabs] Consolidate se23 styles and logic (#10231) [v12] Update changesets (#10232) [LegacyFilters] Consolidate se23 styles and logic (#10217) [Banner] Change status prop to tone (#10206) Replace icon highlight color in box stories (#10226) [TopBar] Consolidate se23 styles and logic (#10221) [Filters] consolidate se23 logic and styles (#10178) [Icon] Update props (#10220) [ButtonGroup] Update variant plain prop name (#10222) [Button] Remove connectedDisclosure (#10182) [LegacyCard] Consolidate se23 styles and logic (#10207) [Navigation] Consolidate se23 logic and styles (#10213) [Frame][ContextualSaveBar] Consolidate se23 styles and logic (#10196) [OptionList] consolidate se23 styles and logic (#10177) [Page] Consolidate se23 styles and logic (#10187) [DataTable] Consolidate conditional logic (#10169) [Thumbnail] Consolidate conditional logic (#10171) [ResourceItem] Consolidate conditional logic (#10172) [FullscreenBar] Consolidate conditional logic (#10173) ...

### WHY are these changes introduced? Fixes #9953 Fixes #10197 ### WHAT is this pull request doing? - `OptionList` consolidate se23 logic - `OptionList` consolidate se23 styles - Remove custom `Checkbox` component in favour of the standard Polaris Checkbox - Removes the `optionRole` prop from the `OptionList` component. This is presently being used to toggle the `presentation` role on the custom checkbox input. This PR removes this prop for a few reasons: - Its no longer used, since the custom Checkbox has been deleted. - The functionality it was providing (toggling presentation role) doesn't seem to be [supported by browsers](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/presentation_role). The browser ignores the `presentation` role on all focusable elements (including inputs). See excerpt from MDN below - A cursory [Grokt](https://grokt.shopify.io/results?q=optionRole) search, rules out internal usage of this prop ![Screenshot 2023-08-23 at 10 16 07 am](https://github.com/Shopify/polaris/assets/12119389/fbf6718b-ad35-46ee-ad00-6c79840ff02b) ### How to 🎩 - Storybook - Prod storybook ### 🎩 checklist - [ ] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [ ] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [ ] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [ ] Updated the component's `README.md` with documentation changes - [ ] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide

### WHY are these changes introduced? Resolves #10035. Updates existing `tall-chickens-repeat` changeset with all the component consolidation work (included any in progress/ready for review/up next/backlog PRs as well). Adds changesets for [OptionList](#10177) `optionRole` and [Page](#10187) `divider` prop removal.

### WHY are these changes introduced? Fixes Shopify#9953 Fixes Shopify#10197 ### WHAT is this pull request doing? - `OptionList` consolidate se23 logic - `OptionList` consolidate se23 styles - Remove custom `Checkbox` component in favour of the standard Polaris Checkbox - Removes the `optionRole` prop from the `OptionList` component. This is presently being used to toggle the `presentation` role on the custom checkbox input. This PR removes this prop for a few reasons: - Its no longer used, since the custom Checkbox has been deleted. - The functionality it was providing (toggling presentation role) doesn't seem to be [supported by browsers](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/presentation_role). The browser ignores the `presentation` role on all focusable elements (including inputs). See excerpt from MDN below - A cursory [Grokt](https://grokt.shopify.io/results?q=optionRole) search, rules out internal usage of this prop ![Screenshot 2023-08-23 at 10 16 07 am](https://github.com/Shopify/polaris/assets/12119389/fbf6718b-ad35-46ee-ad00-6c79840ff02b) ### How to 🎩 - Storybook - Prod storybook ### 🎩 checklist - [ ] Tested on [mobile](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting.md#cross-browser-testing) - [ ] Tested on [multiple browsers](https://help.shopify.com/en/manual/shopify-admin/supported-browsers) - [ ] Tested for [accessibility](https://github.com/Shopify/polaris/blob/main/documentation/Accessibility%20testing.md) - [ ] Updated the component's `README.md` with documentation changes - [ ] [Tophatted documentation](https://github.com/Shopify/polaris/blob/main/documentation/Tophatting%20documentation.md) changes in the style guide

gwyneplaine force-pushed the v12/9953 branch from 82a4bfd to 994cf40 Compare August 23, 2023 00:18

gwyneplaine commented Aug 23, 2023

View reviewed changes

gwyneplaine force-pushed the v12/9953 branch 6 times, most recently from 4507a3a to 876aa12 Compare August 23, 2023 04:19

gwyneplaine linked an issue Aug 23, 2023 that may be closed by this pull request

[OptionList] Consolidate se23 logic and styles #9953

Closed

gwyneplaine requested review from sophschneider, laurkim, jesstelford and sam-b-rose August 23, 2023 04:43

laurkim approved these changes Aug 23, 2023

View reviewed changes

sophschneider approved these changes Aug 23, 2023

View reviewed changes

gwyneplaine added 2 commits August 24, 2023 10:58

OptionList consolidate se23 logic

d64653d

OptionList consolidate se23 styles

8df8d65

gwyneplaine force-pushed the v12/9953 branch from 876aa12 to 8df8d65 Compare August 24, 2023 00:58

gwyneplaine linked an issue Aug 24, 2023 that may be closed by this pull request

OptionList - Remove optionRole prop #10197

Closed

gwyneplaine mentioned this pull request Aug 24, 2023

OptionList - Remove optionRole prop #10197

Closed

gwyneplaine merged commit 9621658 into next Aug 24, 2023
13 checks passed

gwyneplaine deleted the v12/9953 branch August 24, 2023 01:21

laurkim mentioned this pull request Aug 25, 2023

[v12] Update changesets #10232

Merged

sam-b-rose mentioned this pull request Sep 19, 2023

Remove nav level rule #10568

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OptionList] consolidate se23 styles and logic #10177

[OptionList] consolidate se23 styles and logic #10177

gwyneplaine commented Aug 23, 2023 •

edited

Loading

gwyneplaine Aug 23, 2023

gwyneplaine Aug 23, 2023

gwyneplaine Aug 23, 2023

sophschneider Aug 23, 2023

laurkim Aug 23, 2023 •

edited

Loading

laurkim Aug 23, 2023

gwyneplaine commented Aug 23, 2023

laurkim commented Aug 23, 2023 •

edited

Loading

laurkim left a comment

sophschneider left a comment

sophschneider Aug 23, 2023

sophschneider Aug 23, 2023

		);

		const checkBoxRole = role === 'option' ? 'presentation' : undefined;

[OptionList] consolidate se23 styles and logic #10177

[OptionList] consolidate se23 styles and logic #10177

Conversation

gwyneplaine commented Aug 23, 2023 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

gwyneplaine Aug 23, 2023

Choose a reason for hiding this comment

gwyneplaine Aug 23, 2023

Choose a reason for hiding this comment

gwyneplaine Aug 23, 2023

Choose a reason for hiding this comment

sophschneider Aug 23, 2023

Choose a reason for hiding this comment

laurkim Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

laurkim Aug 23, 2023

Choose a reason for hiding this comment

gwyneplaine commented Aug 23, 2023

laurkim commented Aug 23, 2023 • edited Loading

laurkim left a comment

Choose a reason for hiding this comment

sophschneider left a comment

Choose a reason for hiding this comment

sophschneider Aug 23, 2023

Choose a reason for hiding this comment

sophschneider Aug 23, 2023

Choose a reason for hiding this comment

gwyneplaine commented Aug 23, 2023 •

edited

Loading

laurkim Aug 23, 2023 •

edited

Loading

laurkim commented Aug 23, 2023 •

edited

Loading