-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: initial commit for select-box component #7287
base: main
Are you sure you want to change the base?
Conversation
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.
thanks! real quick review
}, | ||
gridTemplateColumns: { | ||
orientation: { | ||
horizontal: '48px 1fr' |
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 take an array which can handle scaling values, see Menu
M: size(2) | ||
}, | ||
vertical: size(8) |
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.
2 and 8 should not need size
I think I saw some other values above which you should be able to just pass along
https://github.com/adobe/react-spectrum/blob/main/packages/%40react-spectrum/s2/style/spectrum-theme.ts#L121
}); | ||
|
||
const selectBoxIconStyle = style({ | ||
color: { |
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.
see
const icon = style<InlineStylesProps>({ |
} | ||
}, | ||
right: 16, | ||
visibility: { |
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.
what about when focused?
horizontal: '50%' | ||
} | ||
}, | ||
right: 16, |
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.
just a heads up, i think this is getting moved to top left following how cards looks
https://github.com/adobe/react-spectrum/blob/main/packages/%40react-spectrum/s2/src/Card.tsx#L202
may also be worthwhile to compare styles from cards to this, I suspect there's a fair amount of overlap
|
||
return ( | ||
<AriaSelector | ||
className={renderProps => selectBoxStyle({...renderProps, orientation, 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.
We need to include UNSAFE classname/style and style overrides, see
let {density = 'regular', size = 'M', variant = 'primary', UNSAFE_className = '', UNSAFE_style, styles, id, ...otherProps} = props; |
|
||
export {unwrapValue, ensureArray}; | ||
|
||
export type SelectBoxValueType = string | 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.
i think we can simplify this to always an array, that's how our other components that can switch single/multi select work
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.
@snowystinger do you have guidance/recommendations for whether this component should be using ValueBase
for props value
and onChange
or a selection for props selectedKeys
and selectionMode
?
Thanks for the quick review! I'll start making some changes |
FYI, we have some new docs coming which may help with styling https://reactspectrum.blob.core.windows.net/reactspectrum/fa664b0028506a94bdee4bed18562b73b20129d0/storybook-s2/index.html?path=/docs/style-macro--docs |
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.
Approving this one to go in. I've removed it from the index file, so it won't go out in next release.
Further work will be done in followup branches. I'll compile a list of feedback today or tomorrow to assist in that.
Initial commit for a Select box component.
Some details were missing in the designs so I took some liberties in some of the styles and props. Mostly want to get this out and start collecting feedback
✅ Pull Request Checklist:
📝 Test Instructions:
Added a SelectBoxGroup story in the s2 storybook
🧢 Your Project:
Adobe