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

Try Ariakit Select for new CustomSelectControl component #55790

Merged
merged 36 commits into from
Nov 23, 2023

Conversation

brookewp
Copy link
Contributor

@brookewp brookewp commented Nov 2, 2023

Note

WIP: This component is experimental and will have many upcoming iterations. Please feel free to add any suggestions to the above tracking issue.

What?

Part of #55023

Try creating a new CustomSelectControl using Ariakit to include new features, including an option to render children.

Why?

For more customization potential and a more intuitive API, allowing the use of JSX to add select values rather than a strict object like in our current CustomSelectControl component.

How?

By creating a compound component:

<CustomSelect>
	<CustomSelectItem value="Small">
		<span style={ { fontSize: '75%' } }>Small</span>
	</CustomSelectItem>
	<CustomSelectItem value="Something bigger">
		<span style={ { fontSize: '200%' } }>Something bigger</span>
	</CustomSelectItem>
</CustomSelect>

New Features

Multi-Select

Option to select more than one value:

Screenshot 2023-11-20 at 11 05 59 AM

To render any JSX children, with rich previews:

Screenshot 2023-11-20 at 11 06 14 AM

Testing Instructions

  1. npm run storybook: dev
  2. Test out the component and ensure it works as expected as a select, with the new features mentioned above

Generally:

  • Styles should match existing CustomSelectControl as closely as possible
  • Selecting a value changes the input

Per Story:

  • Default: Padding, font-size, and height should change for size small
  • Multi Select: Should be able to select and deselect multiple items and count in input should reflect this

Follow-ups:

@brookewp brookewp added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Nov 2, 2023
@brookewp brookewp self-assigned this Nov 2, 2023
@brookewp brookewp force-pushed the try/customselect-ariakit branch 2 times, most recently from d5b1287 to 72c6e34 Compare November 7, 2023 23:24
Copy link

github-actions bot commented Nov 7, 2023

Flaky tests detected in 3edc792.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6830589057
📝 Reported issues:

@brookewp brookewp changed the title [WIP] Try Ariakit Select for new CustomSelectControl component Try Ariakit Select for new CustomSelectControl component Nov 10, 2023
@brookewp brookewp marked this pull request as ready for review November 20, 2023 19:17
@brookewp brookewp requested review from a team and ciampo November 20, 2023 19:17
@jameskoster
Copy link
Contributor

Should the design for CustomSelectItem be inspired by the DropdownMenu work? Specifically; placing the checkmark icon on the left?

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

Thanks for working on it @brookewp!

I've left some thoughts and questions as I took a quick first look, let me know what you think.

Also, I think it would be nice to add some unit tests.

@ciampo
Copy link
Contributor

ciampo commented Nov 21, 2023

Should the design for CustomSelectItem be inspired by the DropdownMenu work? Specifically; placing the checkmark icon on the left?

Hey @jameskoster , that is a good suggestion! For now, this new version of the component is still quite experimental, so maybe we won't apply any changes in this PR specifically. But we should definitely take this opportunity to take a look at CustomSelectControl and make tweaks were necessary (including the checkmark icon).

@brookewp , we should probably add a task about reviewing the design of the component in the main tracking issue ?

@tyxla

I've left some thoughts and questions as I took a quick first look, let me know what you think.

Also, I think it would be nice to add some unit tests.

@brookewp and I recently discussed the approach to this PR, and decided that, since this new version of the component isn't used anywhere, isn't exported, and is temporary, we could adopt a more iterative approach with the PRs, meaning that we were planning on wrapping this PR up even if the work on the component is clearly still in progress.

For example, Brooke was going to work on adding unit tests to the legacy CustomSelectControl component first, and then use them on this new version of the component to spot differences / regressions.

As explained above in the reply to Jay, styles for now are also a "best effort" to mimic the legacy component, and they will likely be revised at a later point in the process.

Finally, we can also ignore the README file for now, since it would be a bit of a wasted effort to keep it up to date with the component's prop types (while those types are still WIP).

@brookewp is also going to update the tracking issue with more details about all the more granular follow-up tasks, to that we can better keep track of the work that is being done.

@brookewp
Copy link
Contributor Author

brookewp commented Nov 21, 2023

Thank you all for the suggestions! It is helpful to get this initial feedback to keep it in mind as we work on refining the component in upcoming iterations. In addition to the follow-ups I've added to the description, I've also included a note on this and it being experimental.

I'll also be adding those follow-ups and suggestions mentioned here to the tracking issue #55023, but comments are also welcome from anyone there, too.

@brookewp brookewp requested a review from ciampo November 21, 2023 23:37
Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

I left a few more comments, but I believe that we're good to merge this initial WIP once they've been addressed 🚀

@brookewp brookewp merged commit 427fa76 into trunk Nov 23, 2023
50 checks passed
@brookewp brookewp deleted the try/customselect-ariakit branch November 23, 2023 22:13
@github-actions github-actions bot added this to the Gutenberg 17.2 milestone Nov 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
Status: Done 🎉
Development

Successfully merging this pull request may close these issues.

7 participants