-
Notifications
You must be signed in to change notification settings - Fork 1.3k
wip: S2 SelectBox Implementation #8541
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
base: main
Are you sure you want to change the base?
Conversation
seems like there's a couple of lint errors, mind running |
}, | ||
orientation: { | ||
horizontal: { | ||
size: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since the value is auto
for all the sizes, you can just do:
orientation: {
horizontal: 'auto'
}
}, | ||
orientation: { | ||
horizontal: { | ||
size: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
}, | ||
position: 'relative', | ||
borderWidth: 2, | ||
borderStyle: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
borderStyle: { | |
borderStyle: 'solid' |
borderColor: { | ||
default: 'transparent', | ||
isSelected: 'gray-900', | ||
isFocusVisible: 'transparent' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this needs to go above isSelected
since selection should have precedence
allowMultiSelect = false, | ||
size = 'M', | ||
orientation = 'vertical' | ||
} = groupContext; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can combine this with line 167 since groupContext
isnt used anywhere else
* The size of the SelectBoxGroup. | ||
* @default 'M' | ||
*/ | ||
size?: 'S' | 'M' | 'L' | 'XL', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to support XS
since it's defined in the style macro above
allowMultiSelect: boolean, | ||
children: ReactNode, | ||
style?: React.CSSProperties, | ||
className?: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style
and className
might need the UNSAFE_
prefix. and you might want to add the styles
prop
|
||
useEffect(() => { | ||
if (value !== undefined) { | ||
onSelectionChange(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rather than call this in a useEffect, create a callback which set on onChange
, and in that callback, set the value as well as calling onSelectionChange
@@ -69,6 +69,8 @@ export {Provider} from './Provider'; | |||
export {Radio} from './Radio'; | |||
export {RadioGroup, RadioGroupContext} from './RadioGroup'; | |||
export {RangeCalendar, RangeCalendarContext} from './RangeCalendar'; | |||
export {SelectBox} from './SelectBox'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alphabetize
@@ -146,6 +148,8 @@ export type {ProgressCircleProps} from './ProgressCircle'; | |||
export type {ProviderProps} from './Provider'; | |||
export type {RadioProps} from './Radio'; | |||
export type {RadioGroupProps} from './RadioGroup'; | |||
export type {SelectBoxProps} from './SelectBox'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alphabetize
label?: ReactNode | ||
} | ||
|
||
const SelectorGroup = forwardRef<HTMLDivElement, SelectorGroupProps>(function SelectorGroupComponent({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please move this component below the other one so that the file reads as
Top level followed by children/nested
|
||
// Simple basic styling with proper dark mode support | ||
const selectBoxStyles = style({ | ||
...focusRing(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
const StarIcon = createIcon(StarSVG); | ||
|
||
const meta: Meta<typeof SelectBoxGroup> = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename this file to SelectBoxGroup.stories
justifyContent: 'center', | ||
flexShrink: 0, | ||
alignItems: 'center', | ||
fontFamily: 'sans', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since font
is already a shorthand for a bunch of other font properties, you don't need to set fontFamily
since font
already handles it
* The size of the SelectBoxGroup. | ||
* @default 'M' | ||
*/ | ||
size?: 'XS' | 'S' | 'M' | 'L' | 'XL', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we support XS? checkbox doesn't even have an XS size. i think the checkbox looks too close to the icon(?) when the select box is XS, especially compared to the other sizes.
this also might be good to clarify with design because in the S1 design docs, under Size, the image shows XS but then in the text they say they support four sizes: small, medium, large, and extra-large
<> | ||
{(renderProps.isSelected || renderProps.isHovered || renderProps.isFocusVisible) && ( | ||
<div className={checkboxContainer({...renderProps, size}, props.styles)}> | ||
<Checkbox |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one thing i noticed is that if i have selectionMode='single'
, hover over a SelectBox, and click on the checkbox, nothing happens. however, i would expect that the checkbox be marked as selected.
does this need to be a CheckBox component or can we just style something to look like a checkbox? i don't think having nested label elements is valid. it might cause issues with forms. but also right now, i can keyboard navigate to both the select box and the checkbox inside of it. i'm not sure if that's expected or not
ref | ||
}; | ||
|
||
return allowMultiSelect ? ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
part of me wonders if this should be built using Checkbox/Radios. i say that because the keyboard experience is kinda interesting? for example, it's layout is sorta grid-like, so i would expect that if i arrow down, i would go to the item below but sometimes i go the item to the right due to the nature of how Checkbox/Radio's work
EDIT: that said, not sure what you would use since collections seems a little overkill for something like this
color: { | ||
isDisabled: 'disabled' | ||
} | ||
}, getAllowedOverrides()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think you need getAllowedOverrides
here since these aren't styles that can be overridden or changed by the user. the same can be said for textContainer
, descriptionText
, and contentContainer
return ( | ||
<> | ||
{(renderProps.isSelected || renderProps.isHovered || renderProps.isFocusVisible) && ( | ||
<div className={checkboxContainer({...renderProps, size}, props.styles)}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should checkboxContainer take props.styles
?
/** | ||
* Whether the SelectBoxGroup should be displayed with an emphasized style. | ||
*/ | ||
isEmphasized?: boolean, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i can't seem to find the designs for an isEmphasized style...
Closes #8540
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: