Skip to content

Commit

Permalink
[OptionList] consolidate se23 styles and logic (#10177)
Browse files Browse the repository at this point in the history
<!--
  ☝️How to write a good PR title:
- Prefix it with [ComponentName] (if applicable), for example: [Button]
  - Start with a verb, for example: Add, Delete, Improve, Fix…
  - Give as much context as necessary and as little as possible
  - Prefix it with [WIP] while it’s a work in progress
-->

### 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
  • Loading branch information
gwyneplaine authored Aug 24, 2023
1 parent 8755b46 commit 9621658
Show file tree
Hide file tree
Showing 11 changed files with 110 additions and 531 deletions.
88 changes: 20 additions & 68 deletions polaris-react/src/components/OptionList/OptionList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ import {useDeepEffect} from '../../utilities/use-deep-effect';
import {Box} from '../Box';
import type {BoxProps} from '../Box';
import {Text} from '../Text';
import {Bleed} from '../Bleed';
import {useFeatures} from '../../utilities/features';
import {BlockStack} from '../BlockStack';

import {Option} from './components';
Expand All @@ -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;
/** Sections containing a header and related options */
sections?: SectionDescriptor[];
/** The selected options */
Expand All @@ -53,14 +49,12 @@ export function OptionList({
selected,
allowMultiple,
role,
optionRole,
verticalAlign,
onChange,
id: idProp,
onPointerEnterOption,
onFocusOption,
}: OptionListProps) {
const {polarisSummerEditions2023} = useFeatures();
const [normalizedOptions, setNormalizedOptions] = useState(
createNormalizedOptions(options, sections, title),
);
Expand Down Expand Up @@ -127,34 +121,16 @@ export function OptionList({
const optionsMarkup = optionsExist
? normalizedOptions.map(({title, options}, sectionIndex) => {
const isFirstOption = sectionIndex === 0;
const sectionPaddingBlockStart = polarisSummerEditions2023 ? '3' : '4';
const firstOptionBlockStartPadding = polarisSummerEditions2023
? '05'
: '2';
const titleLevel = isFirstOption ? 'h2' : 'h3';
const titleMarkup = title ? (
<Box
paddingBlockStart={
isFirstOption
? firstOptionBlockStartPadding
: sectionPaddingBlockStart
}
paddingInlineStart={
polarisSummerEditions2023 ? '1_5-experimental' : '2'
}
paddingBlockEnd={polarisSummerEditions2023 ? '1' : '2'}
paddingInlineEnd={
polarisSummerEditions2023 ? '1_5-experimental' : '2'
}
paddingBlockStart={isFirstOption ? '05' : '3'}
paddingInlineStart="1_5-experimental"
paddingBlockEnd="1"
paddingInlineEnd="1_5-experimental"
borderColor="border-subdued"
borderBlockStartWidth={
!isFirstOption && !polarisSummerEditions2023 ? '1' : undefined
}
>
<Text
as={polarisSummerEditions2023 ? titleLevel : 'p'}
variant="headingXs"
>
<Text as={titleLevel} variant="headingXs">
{title}
</Text>
</Box>
Expand All @@ -177,72 +153,48 @@ export function OptionList({
select={isSelected}
allowMultiple={allowMultiple}
verticalAlign={verticalAlign}
role={optionRole}
onPointerEnter={handlePointerEnter}
onFocus={handleFocus}
/>
);
});

const option = (
<Bleed
marginBlockStart={
title || polarisSummerEditions2023 ? undefined : '05'
}
<Box
as="ul"
id={`${id}-${sectionIndex}`}
role={role as BoxProps['role']}
>
<Box
as="ul"
id={`${id}-${sectionIndex}`}
role={role as BoxProps['role']}
>
{optionsMarkup}
</Box>
</Bleed>
{optionsMarkup}
</Box>
);

// eslint-disable-next-line no-nested-ternary
const blockStartPadding = isFirstOption
? // eslint-disable-next-line no-nested-ternary
polarisSummerEditions2023
? title
? '1'
: '0'
: undefined
: // eslint-disable-next-line no-nested-ternary
polarisSummerEditions2023
? title
? '05'
? '1'
: '0'
: '2';
: title
? '05'
: '0';

return (
<Box
key={title || `noTitle-${sectionIndex}`}
as="li"
paddingBlockStart={blockStartPadding}
>
{polarisSummerEditions2023 ? (
<BlockStack gap={isFirstOption && sections ? undefined : '0'}>
{titleMarkup}
{option}
</BlockStack>
) : (
<>
{titleMarkup}
{option}
</>
)}
<BlockStack gap={isFirstOption && sections ? undefined : '0'}>
{titleMarkup}
{option}
</BlockStack>
</Box>
);
})
: null;

return (
<Box
as="ul"
role={role as BoxProps['role']}
padding={polarisSummerEditions2023 ? '1_5-experimental' : '2'}
>
<Box as="ul" role={role as BoxProps['role']} padding="1_5-experimental">
{optionsMarkup}
</Box>
);
Expand Down

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

Loading

0 comments on commit 9621658

Please sign in to comment.