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: add clean up to avoid effect firing twice in strict mode #1436

Merged

Conversation

jeanpan
Copy link
Contributor

@jeanpan jeanpan commented Nov 7, 2022

What:

Add clean up function to isInitialMountRef initialization.

Why:

It fixes the bug that when the page load and if the dropdown is not above the content, the dropdown is focused and cases the page scrolled in strict mode.

How:

As react official doc, we should add clean-up function for initialization on effect to avoid the effect firing twice in the strict mode (development mode)

Checklist:

  • Documentation
  • Tests
  • TypeScript Types
  • Flow Types
  • Ready to be merged

@silviuaavram
Copy link
Collaborator

silviuaavram commented Nov 23, 2022

Hi @jeanpan! Thanks for the fix!

Can you run prettier over the code? The build fails because of too many statements, and it can be easily be fixed by prettier since it already has the rule from kcd-scipts.

@jeanpan jeanpan force-pushed the JEANPAN/fix-effect-firing-twice branch from ccbed3c to 04d0b85 Compare November 30, 2022 05:52
@jeanpan
Copy link
Contributor Author

jeanpan commented Nov 30, 2022

@silviuaavram, sorry for the late reply. Updated to fix the lint error. Could you review this again?

@codecov-commenter
Copy link

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (04d0b85) compared to base (04a9d9d).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff            @@
##            master     #1436   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           18        18           
  Lines         1684      1690    +6     
  Branches       503       503           
=========================================
+ Hits          1684      1690    +6     
Impacted Files Coverage Δ
src/hooks/useCombobox/index.js 100.00% <100.00%> (ø)
src/hooks/useMultipleSelection/index.js 100.00% <100.00%> (ø)
src/hooks/useSelect/index.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@silviuaavram silviuaavram merged commit 25a673b into downshift-js:master Dec 17, 2022
@silviuaavram
Copy link
Collaborator

@all-contributors please add @jeanpan for code

@allcontributors
Copy link
Contributor

@silviuaavram

I've put up a pull request to add @jeanpan! 🎉

@github-actions
Copy link

🎉 This PR is included in version 7.0.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants