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

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

Open
ciampo opened this issue May 31, 2022 · 6 comments
Labels
[Feature] Component System WordPress component system [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.

Comments

@ciampo
Copy link
Contributor

ciampo commented May 31, 2022

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:

  • 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
  • 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.
  • with this approach, SelectControl and CustomSelectControl would practically have the same underlying implementation
  • 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)
  • 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?
  • ariakit's Select component uses internally ariakit's Popover — we may ultimately want to switch to that implementation, for consistency and efficiency's sake

TODO:

  • Try adding more "gutenberg"-like styles
  • Look into advanced examples (multiple selection, combobox)
  • Test the component on smaller viewports / mobile devices
  • Look at the docs / code and experiment with all available options
  • Test controlled vs uncontrolled behavior
@ciampo ciampo added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components [Feature] Component System WordPress component system labels May 31, 2022
@noisysocks
Copy link
Member

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: #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
Copy link
Contributor Author

ciampo commented Jun 9, 2022

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 #41430 (comment).

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

noisysocks commented Jun 10, 2022

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

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

mirka commented Aug 16, 2022

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

Just noting that in #44966 I added an __experimentalShowSelectedHint prop to CustomSelectControl which probably can/should be removed once we have something like renderItem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Component System WordPress component system [Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

4 participants