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.

Sign up for GitHub

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

Jump to bottom

CustomSelectControlV2: Use InputBase for styling #60261

Merged
merged 6 commits into from
May 28, 2024
Merged

CustomSelectControlV2: Use InputBase for styling #60261

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
55f5b1f
Stop exporting unconnected InputBase
mirka Mar 27, 2024
b24f0f1
CustomSelectControlV2: Use InputBase for styling
mirka Mar 27, 2024
46eeb44
Adjust styles
mirka Mar 27, 2024
a45ee08
Clean up
mirka Mar 27, 2024
5c58d6c
Add changelog
mirka May 24, 2024
106f28a
Add JSDoc description for InputBase
mirka May 28, 2024
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
73 changes: 37 additions & 36 deletions packages/components/src/custom-select-control-v2/custom-select.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
*/
import { createContext, useMemo } from '@wordpress/element';
import { __, sprintf } from '@wordpress/i18n';
import { Icon, chevronDown } from '@wordpress/icons';

/**
* Internal dependencies
Expand All @@ -14,13 +13,12 @@ import type {
CustomSelectContext as CustomSelectContextType,
CustomSelectStore,
CustomSelectButtonProps,
CustomSelectButtonSize,
_CustomSelectProps,
} from './types';
import {
contextConnectWithoutRef,
useContextSystem,
type WordPressComponentProps,
} from '../context';
import type { WordPressComponentProps } from '../context';
import InputBase from '../input-control/input-base';
import SelectControlChevronDown from '../select-control/chevron-down';

export const CustomSelectContext =
createContext< CustomSelectContextType >( undefined );
Expand All @@ -46,23 +44,19 @@ function defaultRenderSelectedValue(
return value;
}

const UnconnectedCustomSelectButton = (
props: Omit<
WordPressComponentProps<
CustomSelectButtonProps & CustomSelectStore,
'button',
false
>,
'onChange'
>
) => {
const {
renderSelectedValue,
size = 'default',
store,
...restProps
} = useContextSystem( props, 'CustomSelectControlButton' );

const CustomSelectButton = ( {
renderSelectedValue,
size = 'default',
store,
...restProps
}: Omit<
WordPressComponentProps<
CustomSelectButtonProps & CustomSelectButtonSize & CustomSelectStore,
'button',
false
>,
'onChange'
> ) => {
const { value: currentValue } = store.useState();

const computedRenderSelectedValue = useMemo(
Expand All @@ -81,21 +75,18 @@ const UnconnectedCustomSelectButton = (
showOnKeyDown={ false }
>
<div>{ computedRenderSelectedValue( currentValue ) }</div>
<Icon icon={ chevronDown } size={ 18 } />
</Styled.Select>
);
};

const CustomSelectButton = contextConnectWithoutRef(
UnconnectedCustomSelectButton,
'CustomSelectControlButton'
);
Comment on lines -89 to -92
Copy link
Member Author

Choose a reason for hiding this comment

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

We don't need to connect this to the context system anymore.

Copy link
Member

Choose a reason for hiding this comment

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

Would you mind elaborating on why?

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind, already seen #60261 (comment) 👍🏻


function _CustomSelect( props: _CustomSelectProps & CustomSelectStore ) {
function _CustomSelect(
props: _CustomSelectProps & CustomSelectStore & CustomSelectButtonSize
) {
const {
children,
hideLabelFromVision = false,
label,
size,
store,
...restProps
} = props;
Expand All @@ -109,12 +100,22 @@ function _CustomSelect( props: _CustomSelectProps & CustomSelectStore ) {
{ label }
</Styled.SelectLabel>
) }
<CustomSelectButton { ...restProps } store={ store } />
<Styled.SelectPopover gutter={ 12 } store={ store } sameWidth>
<CustomSelectContext.Provider value={ { store } }>
{ children }
</CustomSelectContext.Provider>
</Styled.SelectPopover>
<InputBase
__next40pxDefaultSize
size={ size }
suffix={ <SelectControlChevronDown /> }
>
<CustomSelectButton
{ ...restProps }
size={ size }
store={ store }
/>
<Styled.SelectPopover gutter={ 12 } store={ store } sameWidth>
<CustomSelectContext.Provider value={ { store } }>
{ children }
</CustomSelectContext.Provider>
</Styled.SelectPopover>
</InputBase>
</>
);
}
Expand Down
53 changes: 22 additions & 31 deletions packages/components/src/custom-select-control-v2/legacy-component/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,20 @@
*/
// eslint-disable-next-line no-restricted-imports
import * as Ariakit from '@ariakit/react';
/**
* WordPress dependencies
*/
import { useMemo } from '@wordpress/element';

/**
* Internal dependencies
*/
import _CustomSelect from '../custom-select';
import CustomSelectItem from '../item';
import type { LegacyCustomSelectProps } from '../types';
import * as Styled from '../styles';
import { ContextSystemProvider } from '../../context';

function CustomSelectControl( props: LegacyCustomSelectProps ) {
const {
__experimentalShowSelectedHint,
__next40pxDefaultSize = false,
describedBy,
options,
onChange,
size = 'default',
Expand Down Expand Up @@ -94,39 +91,33 @@ function CustomSelectControl( props: LegacyCustomSelectProps ) {
);
};

// translate legacy button sizing
const contextSystemValue = useMemo( () => {
let selectedSize;

const translatedSize = ( () => {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a good reason for the IIFE here? Can be a simple if without an IIFE IMHO.

Copy link
Member Author

Choose a reason for hiding this comment

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

To me, this takes the least cognitive load to read because it doesn't introduce a mutable variable (let), and it's a bit of overkill to extract out the entire function. I'm probably more mutation-averse than average though 😓

Copy link
Member

Choose a reason for hiding this comment

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

Primitive values cannot be mutated, so we're technically not really mutating anything. Also, the IIFE makes the code a bit more complex to read than if we were using a let. No strong feelings either way, so I'm happy to leave this to your judgment!

Copy link
Member Author

Choose a reason for hiding this comment

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

Touché, I should've said reassignment! I always considered variable reassignment to be a form of mutation (as in mutable state), but I guess those two terms should be differentiated in JS at least.

if (
( __next40pxDefaultSize && size === 'default' ) ||
size === '__unstable-large'
) {
selectedSize = 'default';
} else if ( ! __next40pxDefaultSize && size === 'default' ) {
selectedSize = 'compact';
} else {
selectedSize = size;
return 'default';
}

return {
CustomSelectControlButton: { _overrides: { size: selectedSize } },
Copy link
Member Author

Choose a reason for hiding this comment

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

No more need to tunnel this via context system, because we can directly set the newly exposed size prop on _CustomSelect.

Copy link
Member

@fullofcaffeine fullofcaffeine May 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah I see 👍🏻 (regarding https://github.com/WordPress/gutenberg/pull/60261/files#r1616307373).

};
}, [ __next40pxDefaultSize, size ] );

const translatedProps = {
'aria-describedby': props.describedBy,
Copy link
Member Author

Choose a reason for hiding this comment

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

describedBy wasn't being destructured from the props, and therefore it was still being passed invalidly to _CustomSelect through the rest props. Fixed.

children,
renderSelectedValue: __experimentalShowSelectedHint
? renderSelectedValueHint
: undefined,
...restProps,
};
Comment on lines -117 to -124
Copy link
Member Author

Choose a reason for hiding this comment

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

I replaced this with direct prop assignments on the _CustomSelect component for simplicity.

if ( ! __next40pxDefaultSize && size === 'default' ) {
return 'compact';
}
return size;
} )();

return (
<ContextSystemProvider value={ contextSystemValue }>
<_CustomSelect { ...translatedProps } store={ store } />
</ContextSystemProvider>
<_CustomSelect
aria-describedby={ describedBy }
renderSelectedValue={
__experimentalShowSelectedHint
? renderSelectedValueHint
: undefined
}
size={ translatedSize }
store={ store }
{ ...restProps }
Copy link
Member

Choose a reason for hiding this comment

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

Do we intentionally allow the restProps to override the above props like store or renderSelectedValue? Should these props be declared below the restProps to prevent that from happening?

Copy link
Contributor

Choose a reason for hiding this comment

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

Everything that can't be merged should be overridable, as per the open/closed pattern.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also store or renderSelectedValue are not documented in the public interface for this, so it would be very at-your-own-risk if anyone tried that.

No strong opinion, but I think the general strategy of "keep everything overridable unless there is a specific, strong reason not to" would be fine.

Copy link
Member

Choose a reason for hiding this comment

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

Would be much better if JS supported if expressions :/

>
{ children }
</_CustomSelect>
);
}

Expand Down
18 changes: 10 additions & 8 deletions packages/components/src/custom-select-control-v2/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import styled from '@emotion/styled';
*/
import { COLORS, CONFIG } from '../utils';
import { space } from '../utils/space';
import type { CustomSelectButtonProps } from './types';
import type { CustomSelectButtonSize } from './types';

const ITEM_PADDING = space( 2 );

Expand Down Expand Up @@ -46,7 +46,7 @@ export const Select = styled( Ariakit.Select, {
size,
hasCustomRenderProp,
}: {
size: NonNullable< CustomSelectButtonProps[ 'size' ] >;
size: NonNullable< CustomSelectButtonSize[ 'size' ] >;
hasCustomRenderProp: boolean;
} ) => {
const heightProperty = hasCustomRenderProp ? 'minHeight' : 'height';
Expand Down Expand Up @@ -79,17 +79,15 @@ export const Select = styled( Ariakit.Select, {
align-items: center;
justify-content: space-between;
background-color: ${ COLORS.theme.background };
border: 1px solid ${ COLORS.ui.border };
border-radius: 2px;
border: none;
cursor: pointer;
font-size: ${ CONFIG.fontSize };
width: 100%;
&[data-focus-visible] {
outline-style: solid;
}
&[aria-expanded='true'] {
outline: 1.5px solid ${ COLORS.theme.accent };
outline: none; // handled by InputBase component
}
${ getSize() }
`;
} );
Expand All @@ -98,6 +96,10 @@ export const SelectPopover = styled( Ariakit.SelectPopover )`
border-radius: 2px;
background: ${ COLORS.theme.background };
border: 1px solid ${ COLORS.theme.foreground };
&[data-focus-visible] {
outline: none; // outline will be on the trigger, rather than the popover
}
`;

export const SelectItem = styled( Ariakit.SelectItem )`
Expand Down
31 changes: 16 additions & 15 deletions packages/components/src/custom-select-control-v2/types.ts
View file
Open in desktop
Copy link
Member Author

Choose a reason for hiding this comment

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

I refactored some of these types because the rejiggering work I did in this PR surfaced some mistakes in how we were using them.

Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,19 @@ export type CustomSelectStore = {

export type CustomSelectContext = CustomSelectStore | undefined;

type CustomSelectSize< Size = 'compact' | 'default' > = {
Copy link
Member Author

Choose a reason for hiding this comment

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

For context, the public interface of CustomSelectControlV2 only has these two size variants, whereas the private (internal) CustomSelectButton has three size variants for back compat.

/**
* The size of the control.
*
* @default 'default'
*/
size?: Size;
};

export type CustomSelectButtonSize = CustomSelectSize<
'compact' | 'default' | 'small'
>;

export type CustomSelectButtonProps = {
/**
* An optional default value for the control when used in uncontrolled mode.
Expand All @@ -30,19 +43,13 @@ export type CustomSelectButtonProps = {
renderSelectedValue?: (
selectedValue: string | string[]
) => React.ReactNode;
/**
* The size of the control.
*
* @default 'default'
*/
size?: 'compact' | 'default' | 'small';
/**
* The value of the control when used in uncontrolled mode.
*/
value?: string | string[];
};

export type _CustomSelectProps = {
export type _CustomSelectProps = CustomSelectButtonProps & {
/**
* The child elements. This should be composed of `CustomSelectItem` components.
*/
Expand All @@ -60,14 +67,8 @@ export type _CustomSelectProps = {
};

export type CustomSelectProps = _CustomSelectProps &
Omit< CustomSelectButtonProps, 'size' > & {
/**
* The size of the control.
*
* @default 'default'
*/
size?: Exclude< CustomSelectButtonProps[ 'size' ], 'small' >;
};
CustomSelectButtonProps &
CustomSelectSize;

/**
* The legacy object structure for the options array.
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/input-control/input-base.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ function getUIFlexProps( labelPosition?: LabelPosition ) {
return props;
}

export function InputBase(
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed this unused export, because a consumer can erroneously import this named version instead of the default export which is properly wrapped in a forwardRef().

(This happened to me while working on this PR and it wasted a couple minutes of my life 😇)

Copy link
Member

Choose a reason for hiding this comment

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

Nit, but, what is the current convention for components - should we provide both (named, default) or just one - and if so, it seems default is preferred?

Another approach would be to connect before while assigning to a const and then export as needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. We do indeed have some conventions that are actually for accommodating our current docgen, more than anything else.

Doesn't matter that much for internal components like InputBase, which aren't fed into the docgen.

function InputBase(
props: WordPressComponentProps< InputBaseProps, 'div' >,
ref: ForwardedRef< HTMLDivElement >
) {
Expand Down
Loading