WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.56k stars 4.22k forks source link

Components: explore using `ariakit`'s `Select` component to implement `SelectControl` & related #41466

Open ciampo opened 2 years ago

ciampo commented 2 years ago

Currently, there a few components in the @wordpress/components package that build onto / expand the vanilla HTML select element:

The goal is explore the feasibility of rewriting some, or all of these components using ariakit's Select component` under the hood

Useful links:


Notes so far:

TODO:

noisysocks commented 2 years ago

Summarising some of the thoughts I remember having during our conversation at WCEU 😀

  • ariakit's Select doesn't seem to use a native select element under the hood — this means that current usages of SelectControl where vanilla option and optiongroup elements are being passed as children will not work. This would potentially be a breaking change; to cope with it, we could either keep both the new version and the old version of the component around while we soft deprecate this type of usage, or we could look into a way to automatically detect and "convert" the vanilla HTML elements to the ariakit equivalents

Personally I think it's nice to use a select element when possible as it means that mobile devices show a native picker which feels more native and is consistent across websites. Maybe we can keep both SelectControl and CustomSelectControl? Or automatically switch from select to a custom implementation when renderItem is provided? Or have it as an option?

  • ariakit adopts a modular approach to how components are exported (SelectItem, SelectGroup, SelectGroupLabel, SelectSeparator). If we want to give our customers the freedom of specifying a custom select item layout, we may have to basically re-export each of these components (while adding some custom styles on top). Not necessarily a bad thing per se, but we should be aware of this shift in the APIs.
  • currently, our SelectControl and CustomSelectControl component don't have a way to customize the look of the dropdown itself — should we consider adding adding a "renderValue" prop, used to render a SelectItem both as the dropdown selected item, and each option? (for reference, see the renderValue function in this example)

I think this all makes sense. We already have a need for it: https://github.com/WordPress/gutenberg/pull/41430. I don't know that we should allow too much customisation, though. For example ariakit exports SelectArrow which feels a little too low level for @wordpress/components.

  • should we soft deprecate the "options" prop (currently used to pass data for which options should be displayed in the select), in favour of a more declarative, markup-oriented approach?

I'm on the fence about this one. I do like that right now you can pass whatever you get from getEntityRecords straight into options. I suppose it's not really a big deal to call map() though, and it makes everything very predictable.

ciampo commented 2 years ago

Thank you for summarizing our conversations here!

Personally I think it's nice to use a select element when possible as it means that mobile devices show a native picker which feels more native and is consistent across websites. Maybe we can keep both SelectControl and CustomSelectControl? Or automatically switch from select to a custom implementation when renderItem is provided? Or have it as an option?

This is definitely a good point, and one that I don't have a definitive answer to for now. I'd like to explore both options (and potentially more alternatives) in the upcoming weeks. I'll probably start by trying to use ariakit's components in CustomSelectControl and see how that goes.

I think this all makes sense. We already have a need for it: #41430. I don't know that we should allow too much customisation, though. For example ariakit exports SelectArrow which feels a little too low level for @wordpress/components.

Re. the amount of customisation, I partially replied to this point in https://github.com/WordPress/gutenberg/pull/41430#issuecomment-1151387482.

Re. the specific SelectArrow, I'll need to look more into it, as that component seems to be tied to the opening/closing of the SelectPopover — in that case, we'd need to either re-export it, or to create an equivalent custom component.

I'm on the fence about this one. I do like that right now you can pass whatever you get from getEntityRecords straight into options. I suppose it's not really a big deal to call map() though, and it makes everything very predictable.

Does the data retrieved from getEntityRecords match perfectly the object structure that the options prop expects? In case it doesn't match it perfectly, a developer would still need to map over the data and adapt it to the requirements of the options prop — and at that point, I would argue that it wouldn't be much different from mapping over components in JSX.

Another reason for deprecating options is that we'd need to maintain/expand its "spec" as we add features to the component — an example of this is #41430 , where we tentatively added an __experimentalImage key. I believe that a more declarative/modular approach (passing children) would give more flexibility and better scalability/maintainability in the long term.

Of course, we'd need to support both approaches (options and children) for a while, in order to not introduce breaking changes and to give time to the consumers of the package to adapt their components.

noisysocks commented 2 years ago

Does the data retrieved from getEntityRecords match perfectly the object structure that the options prop expects? In case it doesn't match it perfectly, a developer would still need to map over the data and adapt it to the requirements of the options prop — and at that point, I would argue that it wouldn't be much different from mapping over components in JSX.

I guess what I had in mind is that you give a list of objects to the component and it doesn't really matter what that object is because what happens is completely up to renderItem.

You're right that some mapping is probably inevitable. I'm with you on transitioning to children but keeping options around for BC.

diegohaz commented 2 years ago
  • ariakit's Select doesn't seem to use a native select element under the hood — this means that current usages of SelectControl where vanilla option and optiongroup elements are being passed as children will not work. This would potentially be a breaking change; to cope with it, we could either keep both the new version and the old version of the component around while we soft deprecate this type of usage, or we could look into a way to automatically detect and "convert" the vanilla HTML elements to the ariakit equivalents

As a matter of curiosity, Ariakit does render a native <select> element underneath, but it's a hidden element so it can support native browser autofill and form submission.

I'd keep SelectControl as-is. Using a native select is always better when you don't need a lot of customization.

mirka commented 2 years ago

One thing I noticed while working on #43230 is that the current implementation of CustomSelectControl renders its options popover within the component's root wrapper div, instead of a separate popover slot. This means the popover is constrained by the width of the CustomSelectControl's container, as seen in this usage within a grid:

The Appearance dropdown in the Typography tools panel

This isn't ideal, since a popover should be able to grow its width wider to the edge of the viewport. Something to keep in mind when rewriting the component.

noisysocks commented 2 years ago

Just noting that in https://github.com/WordPress/gutenberg/pull/44966 I added an __experimentalShowSelectedHint prop to CustomSelectControl which probably can/should be removed once we have something like renderItem.