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

feat(coral): use Combobox instead of NativeSelect for selecting topic names #2101

Closed
wants to merge 14 commits into from

Conversation

mathieu-anderson
Copy link
Contributor

@mathieu-anderson mathieu-anderson commented Dec 8, 2023

Linked issue

Resolves: #2099

What kind of change does this PR introduce?

  • Bug fix
  • New feature
  • Refactor
  • Docs update
  • CI update

What is the current behavior?

We use NativeSelect for Topic name fields. There can be many more topic names than is comfortabnle for the native select UI.

What is the new behavior?

  • Add Combobox component to Form
  • Use Combobox for Topic name fields
Screen.Recording.2023-12-08.at.14.02.06.mov

Other information

  • Many tests had to be adjusted to the new component. Notably, the native utils of react-testing-library do not work with Combobox, so we need to imperatively click on the element to access options now
  • A new console.error from downshift has popped up in the test reports. Not sure how to address it, as I'm not even sure how the component is switching from controlled to uncontrolled.
    Screenshot 2023-12-08 at 13 51 24
  • There is a style discrepancy, as Combobox use an actual placeholder instead of an option for the empty state (color of the content differs). This may also introduce a11y concern, as I think the "empty option" was added to improve a11y.

Requirements (all must be checked before review)

  • The pull request title follows our guidelines
  • Tests for the changes have been added (if relevant)
  • The latest changes from the main branch have been pulled
  • pnpm lint has been run successfully

Mathieu Anderson added 6 commits December 8, 2023 13:47
Signed-off-by: Mathieu Anderson <mathieu.anderson@aiven.io>
Signed-off-by: Mathieu Anderson <mathieu.anderson@aiven.io>
Signed-off-by: Mathieu Anderson <mathieu.anderson@aiven.io>
Signed-off-by: Mathieu Anderson <mathieu.anderson@aiven.io>
@mathieu-anderson mathieu-anderson self-assigned this Dec 8, 2023
@mathieu-anderson mathieu-anderson added enhancement New feature or request Frontend Relates to coral (react app) labels Dec 8, 2023
@mathieu-anderson mathieu-anderson marked this pull request as ready for review December 8, 2023 13:19
@programmiri
Copy link
Contributor

A new console.error from downshift has popped up in the test reports. Not sure how to address it, as I'm not even sure how the component is switching from controlled to uncontrolled.

It's this line:

 const baseNativeProps = !value
   ? props
   : { ...omit(props, "placeholder"), value: value };

Combobox does not have use a select element inside, so it does not have this placeholder. You can just pass

          <BaseCombobox
            {...props}

The placeholder was added only for design purposes for select really. So removing it here is fine.

All in all -> I'm not sure if this combox is accessible. I had a hard time figuring out how to use it with a screenreader and I could look at it to help me understand it. I think we should check how that's usually done and recommended and see if we can support users with screen reader better. Otherwise we're improving UX for some user by removing usability for others. The fact that testing library has a problem figuring that element out is probably also not a good sign in that regard.

Signed-off-by: Mathieu Anderson <mathieu.anderson@aiven.io>
@mathieu-anderson
Copy link
Contributor Author

mathieu-anderson commented Dec 11, 2023

The downshift error may come from the fact that we set some of the defaultValues in TopicAlcRequest's useForm to undefined, and then change these values with fields that are wrapped in _Controller, kind of like what this user describes: downshift-js/downshift#478 (comment)

Additionally, there was some wrong copy pasting action in the Form Combobox component.

This commit seeks to address these issues, and I don't see the warnings any more locally. 2b3a266

@mathieu-anderson
Copy link
Contributor Author

Ajh lol @programmiri you commenetd about what I just pushed a commit to fix ^^

@mathieu-anderson
Copy link
Contributor Author

Closing as this warrants more conversation and possibly a11y fixes to the Combobox component.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Frontend Relates to coral (react app)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix(coral): Replace NativeSelect by Combobox for Topic selection fields
2 participants