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

[SelectUnstyled] Create unstyled select (+ hook) #30113

Merged
merged 61 commits into from
Jan 28, 2022

Conversation

michaldudak
Copy link
Member

@michaldudak michaldudak commented Dec 8, 2021

This PR adds a SelectUnstyled component and useSelect hook.

It is pretty much a new component, with a few parts taken over from the existing @mui/material's Select.
The hook is built using the useButton and useListbox hooks.

Closes #30251
Closes #21022

Design highlights:

  • Instead of piggybacking on MenuItem, I created a dedicated OptionUnstyled component to be used as a child of SelectUnstyled.
  • I introduced grouping support that mimics the native select. It's now possible to wrap OptionUnstyleds in an OptionGroupUnstyled. Nested groups are also supported.
  • Due to differences in behavior and internal types between single-selection and multi-selection Select, I decided to create two components: SingleSelectUnstyled and MultiSelectUnstyled. They are, however, exposed through a SelectUnstyled wrapper that renders one of them based on the value of the multiple prop. I am considering exporting just SingleSelectUnstyled (renamed as SelectUnstyled) and MultiSelectUnstyled, similarly to Mantine)
  • value is not converted to string in the onChange handler. It's therefore possible to use objects as values (Select support does't support object value #30066)
  • Popper is used to display the listbox. This solves the problem of disappearing scrollbars ([Select][material-ui] Don't lock the body scroll and make it non-modal select #17353).
  • The listbox now closes when the select loses focus ([Menu] Clicking outside a menu blocks click from bubbling up #11243)
  • Keyboard navigation works similarly to the native select: pressing up/down arrows when Select has focus changes options without opening the listbox.

Features to implement

  • Rendering a native select
  • Text navigation
  • Form control integration

Another thing to consider in the future would be performance. It feels that we should be able to reduce the work going on behind the scenes if perf becomes a problem.

Preview

https://deploy-preview-30113--material-ui.netlify.app/components/selects/#unstyled

@mui-pr-bot
Copy link

mui-pr-bot commented Dec 8, 2021

Details of bundle changes

@material-ui/core: parsed: +21.38% , gzip: +18.11%
@material-ui/unstyled: parsed: +39.80% , gzip: +26.91%

Generated by 🚫 dangerJS against 72a19c4

@mnajdova
Copy link
Member

@mnajdova Thanks for testing this! I believe I corrected all the issues you pointed out, so I'd appreciate if you could take another look at this.

I found another issue - I cannot open the select when pressing Enter (pressing Space works). Also, could we add test cases for the things we noticed that didn't work so far?

Co-authored-by: Siriwat K <siriwatkunaporn@gmail.com>
@mnajdova
Copy link
Member

We should improve the color contrast here -
image

Should we do something about dark theme? The demos look weird (it's not related to just this PR, we can address it later)

The following example shows a select that opens when hovered over or focused.
It can be controlled by a mouse/touch or a keyboard.

{{"demo": "pages/components/selects/UseSelect.js"}}
Copy link
Member

Choose a reason for hiding this comment

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

I would say we should make sure that keyboard navigation still works.

@michaldudak
Copy link
Member Author

We should improve the color contrast here

Done.

Should we do something about dark theme? The demos look weird (it's not related to just this PR, we can address it later

I'll create an issue to review all unstyled demos and to adapt them to the dark theme. I don't think I would use a Theme to detect a mode, though. Perhaps using a .mode-light and .mode-dark classes that are applied on the body element would be a more generic solution.

Comment on lines +94 to +108
const CustomSelect = React.forwardRef(function CustomSelect<TValue>(
props: SelectUnstyledProps<TValue>,
ref: React.ForwardedRef<HTMLUListElement>,
) {
const components: SelectUnstyledProps<TValue>['components'] = {
Root: StyledButton,
Listbox: StyledListbox,
Popper: StyledPopper,
...props.components,
};

return <SelectUnstyled {...props} ref={ref} components={components} />;
}) as <TValue>(
props: SelectUnstyledProps<TValue> & React.RefAttributes<HTMLUListElement>,
) => JSX.Element;
Copy link
Member

@siriwatknp siriwatknp Jan 28, 2022

Choose a reason for hiding this comment

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

No need to consider in this PR, would be nice if we provide a utility type or a factory function for building custom component with TS like:

const CustomSelect = createSelect((props, ref) => {
  return <SelectUnstyled {...props} ref={ref} components={components} />
})

Copy link
Member Author

Choose a reason for hiding this comment

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

It's worth considering, I guess. The need for type casting is unfortunate but I haven't found a way to avoid it so a helper function could help indeed :)

Comment on lines -155 to -158
/**
* @ignore
*/
onClick: PropTypes.func,
Copy link
Member

Choose a reason for hiding this comment

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

is this removed by the script?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, yarn proptypes removed it. When working on the Select I've noticed an issue on how events are handled in ButtonUnstyled and will improve it in a separate PR.

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

Overall, looks good to me. Can't wait to test it out with Joy!

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Well done :)

@michaldudak michaldudak merged commit ec5df5f into mui:master Jan 28, 2022
@michaldudak michaldudak deleted the unstyled-select branch January 28, 2022 12:43
wladimirguerra pushed a commit to wladimirguerra/material-ui that referenced this pull request Feb 2, 2022
Co-authored-by: Siriwat K <siriwatkunaporn@gmail.com>
@mnajdova mnajdova added component: select This is the name of the generic UI component, not the React module! package: base-ui Specific to @mui/base labels Feb 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: select This is the name of the generic UI component, not the React module! package: base-ui Specific to @mui/base
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SelectUnstyled] Create SelectUnstyled and useSelect [SelectUnstyled] Expose headless API (useSelect)
5 participants