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

chore(combobox): refactor to use useCombobox #15904

Conversation

tay1orjones
Copy link
Member

@tay1orjones tay1orjones commented Mar 6, 2024

Closes #14684
Closes #15927

🚧 WIP

@tw15egan
Copy link
Collaborator

We may want to look into fixing #15927 in this PR as well

Copy link

netlify bot commented Mar 15, 2024

Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit ab83b9f
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/66182978a12e5800073c383c
😎 Deploy Preview https://deploy-preview-15904--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@tay1orjones tay1orjones marked this pull request as ready for review April 9, 2024 16:49
@tay1orjones tay1orjones requested a review from a team as a code owner April 9, 2024 16:49
@tay1orjones
Copy link
Member Author

@tw15egan could you double check that the recent fix #15980 is still correctly implemented here?

@tw15egan
Copy link
Collaborator

@tay1orjones Most of it is there, but it should autofocus the first item in the Combobox when it is opened when no item is selected. When an item is selected, the correct behavior is observed

@tay1orjones
Copy link
Member Author

tay1orjones commented Apr 11, 2024

I've fixed a couple things:

  • I added forwardRef to ListBoxTrigger which got rid of the error failing all the unit tests
  • TJ's note: the first item is highlighted when the menu opens when nothing is selected

A much bigger issue I have yet to track down is that opening the menu and selecting an item with enter doesn't work at all. The selectedItem is never updated. The state reducer example from the downshift docs works properly without having to do anything special. I'm not sure why ours isn't working. I've added some failing conditions to the e2e test that should pass once it's fixed.

Also I think we should double check the stateReducer logic surrounding allowCustomValue. Thanks @tw15egan for outlining how this should work:

Pressing enter should clear the input if allowCustomValue is OFF and the search string does not have any matches. If a user then hit enter, clears since it doesn’t match any options. Any other time it should be matching a value and should select that one.

@tay1orjones
Copy link
Member Author

tay1orjones commented Apr 12, 2024

Two additional items we need to look into::

  • This impacts styling for dropdown - the borders specifically are flashing on the disabled item, also every item looks disabled The fix for this was to reinstate the old behavior of putting disabled on disabled menu items instead of aria-disabled. This avoids any potential breaking changes, and makes it so we don't have to touch styling at all to avoid impacting other components.
  • on hover of a listboxitem, it's applying the focus ring when it shouldn't be - the focus ring should only be present when navigating via keyboard - this is due to [useSelect]: highlightedIndex vs selectedItem downshift-js/downshift#935
    • It appears onmousemove useCombobox is setting the highlighted index to the item currently hovered by the mouse. We need to modify this behavior so the highlighted index only changes if the item is actually selected via click or enter, or if the item has been highlighted via focus by keyboard navigation.

@tay1orjones tay1orjones requested review from tw15egan and removed request for andreancardona April 15, 2024 16:36
dependabot bot and others added 22 commits April 24, 2024 14:00
…arbon-design-system#16163)

Bumps [tar](https://github.com/isaacs/node-tar) from 6.2.0 to 6.2.1.
- [Release notes](https://github.com/isaacs/node-tar/releases)
- [Changelog](https://github.com/isaacs/node-tar/blob/main/CHANGELOG.md)
- [Commits](isaacs/node-tar@v6.2.0...v6.2.1)

---
updated-dependencies:
- dependency-name: tar
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…sign-system#16167)

Bumps [tar](https://github.com/isaacs/node-tar) from 6.1.0 to 6.2.1.
- [Release notes](https://github.com/isaacs/node-tar/releases)
- [Changelog](https://github.com/isaacs/node-tar/blob/main/CHANGELOG.md)
- [Commits](isaacs/node-tar@v6.1.0...v6.2.1)

---
updated-dependencies:
- dependency-name: tar
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…gn-system#16164)

Bumps [tar](https://github.com/isaacs/node-tar) from 6.1.0 to 6.2.1.
- [Release notes](https://github.com/isaacs/node-tar/releases)
- [Changelog](https://github.com/isaacs/node-tar/blob/main/CHANGELOG.md)
- [Commits](isaacs/node-tar@v6.1.0...v6.2.1)

---
updated-dependencies:
- dependency-name: tar
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…n-design-system#16162)

Bumps [tar](https://github.com/isaacs/node-tar) from 6.1.0 to 6.2.1.
- [Release notes](https://github.com/isaacs/node-tar/releases)
- [Changelog](https://github.com/isaacs/node-tar/blob/main/CHANGELOG.md)
- [Commits](isaacs/node-tar@v6.1.0...v6.2.1)

---
updated-dependencies:
- dependency-name: tar
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…sign-system#16161)

Bumps [tar](https://github.com/isaacs/node-tar) from 6.1.0 to 6.2.1.
- [Release notes](https://github.com/isaacs/node-tar/releases)
- [Changelog](https://github.com/isaacs/node-tar/blob/main/CHANGELOG.md)
- [Commits](isaacs/node-tar@v6.1.0...v6.2.1)

---
updated-dependencies:
- dependency-name: tar
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [tar](https://github.com/isaacs/node-tar) from 6.2.0 to 6.2.1.
- [Release notes](https://github.com/isaacs/node-tar/releases)
- [Changelog](https://github.com/isaacs/node-tar/blob/main/CHANGELOG.md)
- [Commits](isaacs/node-tar@v6.2.0...v6.2.1)

---
updated-dependencies:
- dependency-name: tar
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [tar](https://github.com/isaacs/node-tar) from 6.2.0 to 6.2.1.
- [Release notes](https://github.com/isaacs/node-tar/releases)
- [Changelog](https://github.com/isaacs/node-tar/blob/main/CHANGELOG.md)
- [Commits](isaacs/node-tar@v6.2.0...v6.2.1)

---
updated-dependencies:
- dependency-name: tar
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
carbon-design-system#15934)

* fix(ComposedModal): align handleMouseDown between Modal, ComposedModal

* test(ComposedModal): update snaps
…esign-system#16142)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
…ign-system#16172)

Bumps [tar](https://github.com/isaacs/node-tar) from 6.1.0 to 6.2.1.
- [Release notes](https://github.com/isaacs/node-tar/releases)
- [Changelog](https://github.com/isaacs/node-tar/blob/main/CHANGELOG.md)
- [Commits](isaacs/node-tar@v6.1.0...v6.2.1)

---
updated-dependencies:
- dependency-name: tar
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: TJ Egan <[email protected]>
…ign-system#16174)

Bumps [tar](https://github.com/isaacs/node-tar) from 6.1.0 to 6.2.1.
- [Release notes](https://github.com/isaacs/node-tar/releases)
- [Changelog](https://github.com/isaacs/node-tar/blob/main/CHANGELOG.md)
- [Commits](isaacs/node-tar@v6.1.0...v6.2.1)

---
updated-dependencies:
- dependency-name: tar
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…sign-system#16173)

Bumps [tar](https://github.com/isaacs/node-tar) from 6.1.11 to 6.2.1.
- [Release notes](https://github.com/isaacs/node-tar/releases)
- [Changelog](https://github.com/isaacs/node-tar/blob/main/CHANGELOG.md)
- [Commits](isaacs/node-tar@v6.1.11...v6.2.1)

---
updated-dependencies:
- dependency-name: tar
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Guilherme Datilio Ribeiro <[email protected]>
Bumps [vite](https://github.com/vitejs/vite/tree/HEAD/packages/vite) from 5.0.12 to 5.0.13.
- [Release notes](https://github.com/vitejs/vite/releases)
- [Changelog](https://github.com/vitejs/vite/blob/v5.0.13/packages/vite/CHANGELOG.md)
- [Commits](https://github.com/vitejs/vite/commits/v5.0.13/packages/vite)

---
updated-dependencies:
- dependency-name: vite
  dependency-type: direct:development
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@preetibansalui preetibansalui force-pushed the 14684-downshift-usecombobox branch from ee658a5 to 4b0fc38 Compare April 24, 2024 09:21
@tay1orjones tay1orjones requested a review from a team as a code owner April 24, 2024 09:21
@tay1orjones
Copy link
Member Author

Duplicate of #16283

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project