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

feature(packages/ui): Combobox component #14814

Merged
merged 12 commits into from
Oct 25, 2024

Conversation

0xTxbi
Copy link
Member

@0xTxbi 0xTxbi commented Oct 7, 2024

Description

This PR introduces a new Combobox component to our ui package, aimed at improving user experience and accessibility over traditional select elements. It includes a dynamic search functionality, making it easier for users to filter and locate options within large datasets. It also supports full keyboard navigation (enter, Arrow keys, escape, tab) for enhanced accessibility compliance.

Additional features include customizable prepend/append elements, disabled states, and clear visual feedback with check icons, all integrated with our FieldLayout for consistent styling.

Overall, it simplifies dropdown selection.

ScreenGrab

Screenshot 2024-10-07 at 10 12 22

Checklist:

  • 1 PR, 1 purpose: my Pull Request applies to a single purpose
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the docs to reflect my changes if applicable
  • I have added tests (and stories for frontend components) that prove my fix is effective or that my feature works
  • I have performed a self-review of my own code
  • If my code involves visual changes, I am adding applicable screenshots to this thread

@0xTxbi 0xTxbi requested a review from julien51 October 7, 2024 09:17
@cla-bot cla-bot bot added the cla-signed label Oct 7, 2024
@julien51
Copy link
Member

julien51 commented Oct 7, 2024

ok ok, this definetely "looks" good and useful, but I want to make sure it "blends" in our existing style. wdyt?

@0xTxbi
Copy link
Member Author

0xTxbi commented Oct 7, 2024

ok ok, this definetely "looks" good and useful, but I want to make sure it "blends" in our existing style. wdyt?

good question, and yes it does blend with our existing style and branding! do check this demo

@julien51
Copy link
Member

julien51 commented Oct 7, 2024

ok I think we are missing the "more" options here. It is important because it lets us show a smaller number of options by default but if the use r wants more, then more are shown. Do you think you could add that?

@0xTxbi
Copy link
Member Author

0xTxbi commented Oct 7, 2024

ok I think we are missing the "more" options here. It is important because it lets us show a smaller number of options by default but if the use r wants more, then more are shown. Do you think you could add that?

good point! will add that!

@0xTxbi
Copy link
Member Author

0xTxbi commented Oct 9, 2024

and done!

Screenshot 2024-10-09 at 13 06 37

Copy link
Member

@julien51 julien51 left a comment

Choose a reason for hiding this comment

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

Hum, I am not sure I see the moreOptions option.
The goal here is to show a specifc subset of items by default (not just the first n...)

@0xTxbi 0xTxbi requested a review from julien51 October 11, 2024 20:16
@@ -22,7 +22,8 @@ interface Option {
}

interface ComboboxProps {
options: Option[]
initialOptions: Option[]
Copy link
Member

Choose a reason for hiding this comment

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

Let's just keep options here for backward compatibility!

@@ -22,7 +22,8 @@ interface Option {
}

interface ComboboxProps {
options: Option[]
initialOptions: Option[]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
initialOptions: Option[]
options: Option[]

@0xTxbi 0xTxbi requested a review from julien51 October 25, 2024 18:47
// Default story with a mix of initial and more options
export const Default = {
args: {
initialOptions: getRandomItems(allOptions, 5),
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't that be options ?

Copy link
Member

@julien51 julien51 left a comment

Choose a reason for hiding this comment

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

I think the stories need to be updated, no?

packages/ui/lib/components/Form/Combobox.stories.tsx Outdated Show resolved Hide resolved
export const WithLongList = {
args: {
// Generate 10 initial options
initialOptions: getRandomItems(
Copy link
Member

Choose a reason for hiding this comment

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

same?

// Story without any additional options in the "More options" section
export const WithoutMoreOptions = {
args: {
initialOptions: getRandomItems(allOptions, allOptions.length),
Copy link
Member

Choose a reason for hiding this comment

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

ditto

// Story with duplicate options to test handling of non-unique values
export const WithDuplicates = {
args: {
initialOptions: getRandomItems(allOptions, 5),
Copy link
Member

Choose a reason for hiding this comment

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

and there as well?

@0xTxbi
Copy link
Member Author

0xTxbi commented Oct 25, 2024

I think the stories need to be updated, no?

oh definitely. the push failed for some reason!

@0xTxbi 0xTxbi requested a review from julien51 October 25, 2024 19:05
@0xTxbi 0xTxbi merged commit 872d4f1 into unlock-protocol:master Oct 25, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants