feat: S2 SelectBox #8541
feat: S2 SelectBox #8541
Conversation
|
seems like there's a couple of lint errors, mind running |
yihuiliao
changed the title
| allowMultiSelect: boolean, | ||
| children: ReactNode, | ||
| style?: React.CSSProperties, | ||
| className?: string, |
There was a problem hiding this comment.
style and className might need the UNSAFE_ prefix. and you might want to add the styles prop
…id accessibility issues
| import {useDOMRef} from '@react-spectrum/utils'; | ||
| import {useSpectrumContextProps} from './useSpectrumContextProps'; | ||
|
|
||
| export interface SelectBoxGroupProps extends StyleProps { |
There was a problem hiding this comment.
you might want to extend from ListBoxProps from RAC and omit any prop you don't want, or override any props that you might want to change the type of (like selectionMode for example)
|
I made a lot of changes, to bring it more in line with |
| import {useControlledState} from '@react-stately/utils'; | ||
| import {useSpectrumContextProps} from './useSpectrumContextProps'; | ||
|
|
||
| export interface SelectBoxGroupProps<T> extends StyleProps, Omit<ListBoxProps<T>, 'layout' | 'dragAndDropHooks'>, AriaLabelingProps{ |
There was a problem hiding this comment.
can also probably omit renderEmptyState, dependencies, items. it would be nice to also put in the Omit children, selectionMode even though you are just overwriting them here. it helps to make things more clear. i'd go through the rest and see what make sense to keep or leave out.
on top of that, you'll want to add Omit<ListBoxProps, keyof GlobalDOMAttributes` to it
lastly, since ListBoxProps already extends from AriaLabelingProps, you can remove it from here
| const childrenArray = React.Children.toArray(children).filter((x) => x); | ||
| useEffect(() => { | ||
| if (childrenArray.length > 9) { | ||
| console.warn('Invalid content. SelectBoxGroup cannot have more than 9 children.'); |
There was a problem hiding this comment.
discussed with rob, we think this is something the design should be handling, not engineering so we can just remove this warning altogether
| className={renderProps => (props.UNSAFE_className || '') + selectBoxStyles({ | ||
| size, | ||
| orientation, | ||
| isDisabled: renderProps.isDisabled, | ||
| isSelected: renderProps.isSelected, | ||
| isHovered: renderProps.isHovered, | ||
| isFocusVisible: renderProps.isFocusVisible | ||
| }, props.styles)} |
There was a problem hiding this comment.
| className={renderProps => (props.UNSAFE_className || '') + selectBoxStyles({ | |
| size, | |
| orientation, | |
| isDisabled: renderProps.isDisabled, | |
| isSelected: renderProps.isSelected, | |
| isHovered: renderProps.isHovered, | |
| isFocusVisible: renderProps.isFocusVisible | |
| }, props.styles)} | |
| className={renderProps => (props.UNSAFE_className || '') + selectBoxStyles({ | |
| size, | |
| orientation, | |
| ...renderProps | |
| }, props.styles)} |
| selectionMode={selectionMode} | ||
| selectedKeys={selectedKeys} | ||
| onSelectionChange={setSelectedKeys} | ||
| className={UNSAFE_className + gridStyles({gutterWidth, orientation}, props.styles)} |
There was a problem hiding this comment.
| className={UNSAFE_className + gridStyles({gutterWidth, orientation}, props.styles)} | |
| className={(UNSAFE_className || '')+ gridStyles({gutterWidth, orientation}, props.styles)} |
| } | ||
| }); | ||
|
|
||
| const gridStyles = style({ |
There was a problem hiding this comment.
i think you'll need to add outlineStyle: 'none' here, that should help a bit with the focus ring appearing in the isDisabled story
There was a problem hiding this comment.
and turns out the other issue where it will focus on the first item is just a bug in RAC ListBox in general. has to do with the fact that the first item is selected AND disabled. issue doesn't occur when the item isn't initially selected.
if you have time, it would be nice to look into but i wouldn't get too hung up on this since it's pre-existing
| ) | ||
| }; | ||
|
|
||
| export const WithCheckboxes: Story = { |
There was a problem hiding this comment.
i think you can just get rid of this example since you can do this using the controls
| 'illustration . label', | ||
| 'illustration . description' | ||
| ], | ||
| [descriptionOnly]: [ |
There was a problem hiding this comment.
I'm unaware of any description only use cases. I think we can drop this as an unsupported use case
There was a problem hiding this comment.
I have a story that I was using for testing all of the possible layouts. I'll just remove the stuff that has unsupported cases as well.
| slots: { | ||
| label: { |
There was a problem hiding this comment.
add the default slot as a possibility for the label as well. see Menu for an example
| selectionMode={selectionMode} | ||
| selectedKeys={selectedKeys} | ||
| onSelectionChange={setSelectedKeys} | ||
| className={UNSAFE_className + gridStyles({gutterWidth, orientation}, props.styles)} |
There was a problem hiding this comment.
| className={UNSAFE_className + gridStyles({gutterWidth, orientation}, props.styles)} | |
| className={(UNSAFE_className || '') + gridStyles({gutterWidth, orientation}, props.styles)} |
so we don't get 'undefined' in our classname string accidentally
| aria-label="SelectBoxGroup" | ||
| aria-labelledby="SelectBoxGroup" | ||
| style={{ | ||
| gridTemplateColumns: `repeat(${numColumns}, 1fr)`, |
There was a problem hiding this comment.
do we need to limit the columns? could we use css Grid auto-fit instead?
https://css-tricks.com/auto-sizing-columns-css-grid-auto-fill-vs-auto-fit/
Then it won't matter what size container this is rendered into. Instead, item sizes would dictate how many can fit
There was a problem hiding this comment.
This is a feature/holdover from quarry. I wasn't really sure of the justification. I could go either way personally.
There was a problem hiding this comment.
I'd prefer to let it adjust to the container it's in. It's really tough to maintain the layout without overflow edge cases otherwise. You'd have to use container queries and resize observers, which isn't common knowledge yet nor is it great for performance.
@dannify do you have more information here though for requirements?
There was a problem hiding this comment.
team chatted about this yesterday, we feel like we don't need these two options for now and instead the SelectBoxes should fill the available room via what @snowystinger described above. I see you mentioned it is a hold over from quarry, do you know if it is a hard requirement?
There was a problem hiding this comment.
It isn't a hard requirement afaik. I'll be removing it shortly (and the gutterWidths).
| ); | ||
|
|
||
| return ( | ||
| <ListBox |
There was a problem hiding this comment.
think you'll need to spread the remainder of the props here, props like disabledKeys/autoFocus/other Listbox props aren't being applied.
| const gridStyles = style({ | ||
| display: 'grid', | ||
| outline: 'none', |
There was a problem hiding this comment.
strange, I see the default browser outline persist if the selectbox is disabled and autofocused:
| import {useControlledState} from '@react-stately/utils'; | ||
| import {useSpectrumContextProps} from './useSpectrumContextProps'; | ||
|
|
||
| export interface SelectBoxGroupProps<T> extends StyleProps, Omit<ListBoxProps<T>, keyof GlobalDOMAttributes | 'layout' | 'dragAndDropHooks' | 'renderEmptyState' | 'dependencies' | 'items' | 'children' | 'selectionMode'>{ |
There was a problem hiding this comment.
should SelectBoxGroup actually support onAction? If so, clicking on the checkbox doesn't seem to actually toggle selection when selectionBehavior is "toggle" if there is an onAction provided.
I think we would also want the highlightSelection api for S2 instead of React Aria Component's selectionBehavior
There was a problem hiding this comment.
also should we support a empty state aka if the user doesn't provided any options?
There was a problem hiding this comment.
hm i was thinking about the empty state but design suggests that it select box groups work best with 3 - 9 items. i don't think that's a hard rule so entirely possible to have less than 3 but seems to suggest that select box group likely won't be empty but maybe more of a design question
| showCheckbox?: boolean | ||
| } | ||
|
|
||
| export interface SelectBoxProps extends StyleProps { |
There was a problem hiding this comment.
should this extend ListBoxItemProps? At the very least should support id and textValue (for typeahead maybe? not sure if that is something we want for SelectBoxGroup)
There was a problem hiding this comment.
oh I see value is being used as the id, was it intentional that we wanted to change that API? Funnily enough it echos some of the discussion that the team was having earlier for mutli select pickers and comboboxes, but not sure if we wanted to change to using it immediately for SelectBox
| default: 170, | ||
| orientation: { | ||
| horizontal: 368 | ||
| } |
There was a problem hiding this comment.
I assume these values came from the designs? Did they have tokens associated with them? If so, mind using them directly if they exist or adding a comment listing what token name it is so we can track?
| it('supports different gutter widths', () => { | ||
| render( | ||
| <SelectBoxGroup | ||
| aria-label="Gutter width test" | ||
| selectionMode="single" | ||
| onSelectionChange={() => {}} | ||
| selectedKeys={new Set()} | ||
| gutterWidth="compact"> | ||
| <SelectBox value="option1"> | ||
| <Text slot="label">Option 1</Text> | ||
| </SelectBox> | ||
| </SelectBoxGroup> | ||
| ); | ||
| expect(screen.getByRole('listbox')).toBeInTheDocument(); | ||
| }); |
There was a problem hiding this comment.
probably should be a chromatic test, same with the columns one since chromatic is much better suited for testing visual cases like that
| // Navigate to second option | ||
| await userEvent.keyboard('{ArrowDown}'); | ||
|
|
||
| // Check that navigation works by verifying an option has focus | ||
| const option1 = screen.getByRole('option', {name: 'Option 1'}); | ||
| expect(option1).toHaveFocus(); | ||
| }); |
There was a problem hiding this comment.
was this supposed to move focus to option 2?
| consoleSpy.mockRestore(); | ||
| }); | ||
|
|
||
| it('does not warn with valid number of children', () => { |
There was a problem hiding this comment.
I didn't see a warning implemented in SelectBoxGroup, was there supposed to be one?
There was a problem hiding this comment.
Oh I removed it in a previous commit after feedback from @yihuiliao. I'll remove the test.
| // Just verify the listbox has an aria-labelledby attribute | ||
| expect(listbox).toHaveAttribute('aria-labelledby'); | ||
| expect(listbox.getAttribute('aria-labelledby')).toBeTruthy(); |
There was a problem hiding this comment.
hm, seems to intentionally only check for the existence of aria-labelledby, but as noted above the SelectBoxGroup should accept a user defined aria-labelledby/aria-label
| expect(screen.getByText('With description')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('handles different value types', () => { |
There was a problem hiding this comment.
Do we need this test? I think your other tests already cover checking this value case right?
|
|
||
| const gridStyles = style({ | ||
| display: 'grid', | ||
| outline: 'none', |
There was a problem hiding this comment.
| outline: 'none', | |
| outlineStyle: 'none', |
not sure why it doesn't throw an error but it doesn't look like it's applying if you use outline. it has to be outlineStyle
| const noIllustration = ':not(:has([slot=illustration]))'; | ||
| const selectBoxStyles = style({ | ||
| ...focusRing(), | ||
| outlineOffset: { |
There was a problem hiding this comment.
sorry for the back and forth with the focus ring stuff but did you talk to design about it? seems like in the token file that there is an outline offset. not sure if the border is also supposed to be dark. i assume that border during darker is for selection
| onSelectionChange?: (keys: Selection) => void | ||
| } | ||
|
|
||
| export const SelectBoxContext = createContext<SelectBoxContextValue>({orientation: 'vertical'}); |
There was a problem hiding this comment.
do we need to pass {orientation: 'vertical'} when initializing the context? seems like you default to vertical later anyway
also i don't think we need to export this and keep this as an internal context
| @@ -72,6 +72,7 @@ export {RangeCalendar, RangeCalendarContext} from './RangeCalendar'; | |||
| export {RangeSlider, RangeSliderContext} from './RangeSlider'; | |||
| export {SearchField, SearchFieldContext} from './SearchField'; | |||
| export {SegmentedControl, SegmentedControlItem, SegmentedControlContext} from './SegmentedControl'; | |||
| export {SelectBox, SelectBoxContext, SelectBoxGroup} from './SelectBoxGroup'; | |||
There was a problem hiding this comment.
can we remove the SelectBoxContext from being exported? instead, export the SelectBoxGroupContext
| * SelectBox is a single selectable item in a SelectBoxGroup. | ||
| */ | ||
| export function SelectBox(props: SelectBoxProps): ReactNode { | ||
| let {children, value, isDisabled: individualDisabled = false, UNSAFE_style} = props; |
There was a problem hiding this comment.
| let {children, value, isDisabled: individualDisabled = false, UNSAFE_style} = props; | |
| let {children, value, isDisabled: individualDisabled = false, UNSAFE_style, ...otherProps} = props; |
and then you can spread otherProps on ListBoxItem
| <ListBoxItem | ||
| id={value} | ||
| isDisabled={isDisabled} | ||
| textValue={value} |
There was a problem hiding this comment.
i think rather having us set the textValue to value here, we'll have the user apply what the textValue should be. this should be taken care of if you spread otherProps here
| ...renderProps | ||
| }, props.styles)} | ||
| style={UNSAFE_style}> | ||
| {(renderProps) => ( |
There was a problem hiding this comment.
| {(renderProps) => ( | |
| {({isSelected, isDisabled, isHovered}) => ( |
nothing wrong with what you have, just make things a little more readable since you won't have to so renderProps.isSelected, renderProps.isDisabled, etc.
| let { | ||
| children, | ||
| onSelectionChange, | ||
| selectedKeys: controlledSelectedKeys, | ||
| defaultSelectedKeys, | ||
| selectionMode = 'single', | ||
| orientation = 'vertical', | ||
| numColumns = 2, | ||
| gutterWidth = 'default', | ||
| isDisabled = false, | ||
| showCheckbox = false, | ||
| UNSAFE_className, | ||
| UNSAFE_style | ||
| } = props; |
There was a problem hiding this comment.
| let { | |
| children, | |
| onSelectionChange, | |
| selectedKeys: controlledSelectedKeys, | |
| defaultSelectedKeys, | |
| selectionMode = 'single', | |
| orientation = 'vertical', | |
| numColumns = 2, | |
| gutterWidth = 'default', | |
| isDisabled = false, | |
| showCheckbox = false, | |
| UNSAFE_className, | |
| UNSAFE_style | |
| } = props; | |
| let { | |
| children, | |
| onSelectionChange, | |
| selectedKeys: controlledSelectedKeys, | |
| defaultSelectedKeys, | |
| selectionMode = 'single', | |
| orientation = 'vertical', | |
| numColumns = 2, | |
| gutterWidth = 'default', | |
| isDisabled = false, | |
| showCheckbox = false, | |
| UNSAFE_className, | |
| UNSAFE_style, | |
| ...otherProps | |
| } = props; |
then you can spread them on ListBox like Daniel suggested
Closes #8540
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: