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

feat: initial commit for select-box component #7287

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

dgrossen
Copy link
Collaborator

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:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Added a SelectBoxGroup story in the s2 storybook

🧢 Your Project:

Adobe

@dgrossen dgrossen changed the title initial commit for select-box component feat: initial commit for select-box component Oct 29, 2024
Copy link
Member

@snowystinger snowystinger left a 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'
Copy link
Member

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

Comment on lines +167 to +169
M: size(2)
},
vertical: size(8)
Copy link
Member

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: {
Copy link
Member

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>({
for how to generally color icons

}
},
right: 16,
visibility: {
Copy link
Member

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,
Copy link
Member

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})}
Copy link
Member

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[];
Copy link
Member

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

Copy link
Collaborator Author

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?

@dgrossen
Copy link
Collaborator Author

thanks! real quick review

Thanks for the quick review! I'll start making some changes

@snowystinger
Copy link
Member

snowystinger
snowystinger previously approved these changes Nov 4, 2024
Copy link
Member

@snowystinger snowystinger left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants