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

[listbox] add dynamic content example to reproduce bug with keyboard selection order. #899

Merged
merged 1 commit into from
Jul 14, 2022

Conversation

myrjola
Copy link
Contributor

@myrjola myrjola commented Jan 13, 2022

These changes add an example to reproduce a bug that is fixed by #827

I noticed a bug in the Listbox component when adding ListboxOptions dynamically to it. The keyboard selection order is scrambled. Luckily, there's an open PR to fix the bug.

To reproduce:

  1. Navigate to new example in Storybook http://localhost:9001/?path=/story/listbox--dynamic-content-ts
  2. Click two times on the "Add one more guest" button. This adds two more options to the listbox.
  3. Click the listbox open, the "1 room" options should be selected
  4. Press the "down arrow" key, I expected "2 rooms" to be selected, but instead "3 rooms" is selected.

Thank you for contributing to Reach UI! Please fill in this template before submitting your PR to help us process your request more quickly.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code (Compile and run).
  • Add or edit tests to reflect the change (Run with yarn test).
  • Add or edit Storybook examples to reflect the change (Run with yarn start).
  • Ensure formatting is consistent with the project's Prettier configuration.
  • Add documentation to support any new features.

This pull request:

  • Creates a new package
  • Fixes a bug in an existing package
  • Adds additional features/functionality to an existing package
  • Updates documentation or example code
  • Other

If creating a new package:

  • Make sure the new package directory contains each of the following, and that their structure/formatting mirrors other related examples in the project:
    • examples directory
    • src directory with an index.tsx entry file
    • At least one example file per feature introduced by the new package
    • Base styles in a style.css file (if needed by the new package)

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 13, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit be0bacb:

Sandbox Source
reach-ui-template Configuration

@chaance
Copy link
Member

chaance commented Jul 13, 2022

@myrjola Mind rebasing this on top of the current dev branch? Really I just need this example moved into the new playground package, as package examples are just symlinked now. Thanks!

@chaance chaance added the Status: Awaiting Response Requested and awaiting a response from the issue creator label Jul 13, 2022
@myrjola myrjola force-pushed the listbox-dynamic-content-example branch from 1453bf8 to be0bacb Compare July 14, 2022 07:15
@myrjola
Copy link
Contributor Author

myrjola commented Jul 14, 2022

@chaance I rebased the changes and verified that the bug still exists. Hopefully the fix proposed in #827 will be merged soon.

Here's the reproduction instructions:

  1. Navigate to http://localhost:9001/?path=/story/listbox--dynamic
  2. Click "Add one more guest" button twice
  3. Pressing down arrow from the first item selects the third item when we expected the second item to be selected.

imageimage

@chaance chaance merged commit 6760558 into reach:dev Jul 14, 2022
@chaance chaance removed the Status: Awaiting Response Requested and awaiting a response from the issue creator label Jul 14, 2022
chaance pushed a commit that referenced this pull request Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants