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

[OptionList] consolidate se23 styles and logic #10177

Merged
merged 2 commits into from
Aug 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@laurkim laurkim Aug 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/** 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