CustomSelectControl: refactor using ariakit #41726
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +44 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
noisysocks
reviewed
Jun 15, 2022
|
|
||
| // TODO: | ||
| // - should we use 'select' instead of `div` for props inheritance? | ||
| // - should we allow polymorphism ? |
There was a problem hiding this comment.
I wouldn't really expect CustomSelectControlProps to inherit from HTMLSelectElement at all since it's completely custom and doesn't ultimately render a <select>, no? That is, I'd expect props to have type SelectControlProps and not WordPressComponentProps< SelectControlProps, 'select', false >.
There was a problem hiding this comment.
I wouldn't really expect
CustomSelectControlPropsto inherit fromHTMLSelectElementat all since it's completely custom and doesn't ultimately render a<select>, no?
This in an interesting one. On one end, we should aim at CustomSelectControlProps to be semantically a replacement for the select HTML element — on the other end, it's not going to be easy to support all props/functionality of the native select HTML element.
For example, Ariakit doesn't seem to support all of those props either:
That is, I'd expect props to have type SelectControlProps and not WordPressComponentProps< SelectControlProps, 'select', false >.
By default, components in the package should use the WordPressComponentProps type utility unless there's a good reason for them not to. At most, if we wanted to keep it as general as possible, we could type the props as WordPressComponentProps< SelectControlProps, 'div', false >
1ecba05 to
213d63e
Compare
diegohaz
reviewed
Jun 30, 2022
This was referenced Jul 14, 2022
213d63e to
43e9559
Compare
13 tasks
|
Yup! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Related to #41466
Related to #41430 (comment)
Related to #43475
Likely superseeds #37272
This PR attempts to refactor the
CustomSelectControlcomponent using theSelectcomponents fromariakitWhy?
Using
ariakitunder the hood will allow us to:ariakitlibraryselectelementSee #41466 for more details.
How?
ariakitoptionsvs passingchildren) (also see this comment)CustomSelectControlcomponentonHighlightedIndexChangeor similar proprenderItemprop?)optionsprop?)CustomSelectControlfrom old to new componentTesting Instructions
TBD
Screenshots or screencast
TBD