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

fix(dropdown): close dropdown when item is selected #392

Merged
merged 7 commits into from
Oct 19, 2022
Merged

fix(dropdown): close dropdown when item is selected #392

merged 7 commits into from
Oct 19, 2022

Conversation

ShaneMaglangit
Copy link
Contributor

@ShaneMaglangit ShaneMaglangit commented Oct 17, 2022

Description

Closes the dropdown floating menu whenever a DropdownItem is clicked.

Inside Dropdown/index.tsx, a mapping function is executed that extends the onClick props of all instances of DropdownItem. The extended lines trigger a local state change to closeRequestKey that is passed to the Floating/index.tsx component.

setOpen(false) is called inside the Floating component whenever the closeRequestKey is changed to a unique string generated by the UUID helper function.

This emulates the Dropdown component requesting Floating to close itself. This is the only workaround that I can think of due to how the component is designed. Floating is contained with Dropdown, preventing other ways of passing control of the open state to Dropdown and its child.

We can move this entirely in Floating/index.tsx to perform the mapping but since it is reused on other components, I opted to isolate most changes on the Dropdown components themselves.

Fixes #349

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Manual test through StoryBook

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

Close the dropdown menu whenever an instance of DropdownItem is clicked.

fix #349
… callback

Add comment to the method that attaches clsoeDropdown callback
Adds a tests to check that the dropdown is closed whenever an item is clicked.

fix #349
…ng an additional prop

Remove the additional closeDropdown props from DropdownItem, extend onClick props instead.

fix #349
Fix prettier build error for Dropdown.spec.ts
Updates the Dropdown attachCloseListener comment to better reflect the changes
@ShaneMaglangit
Copy link
Contributor Author

ShaneMaglangit commented Oct 17, 2022

Hey, I would appreciate a little bit of help here. I'm not sure why the test is failing when it passes when tested locally.

The weird part is, the tests pass too when I create a PR to the main branch of my fork.

@rluders
Copy link
Collaborator

rluders commented Oct 17, 2022

Hey, I would appreciate a little bit of help here. I'm not sure why the test is failing when it passes when tested locally.

Well, it is also passing locally for me. Maybe some configuration/restriction to the CI? But... I don't actually see any big difference from other tests. 🤔

@ShaneMaglangit
Copy link
Contributor Author

Ugh, I don't really want to spam this with commits. Let me try following React's warning about using act for these sorts of tests.

Wrap dropdown's mouse interaction tests for item click with act
@rluders rluders merged commit 05f5534 into themesberg:main Oct 19, 2022
@ShaneMaglangit ShaneMaglangit deleted the fix/dropdown-close-select branch October 20, 2022 05:55
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.

Dropdown don't close on select
2 participants