Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

fix(Popup): close previous popup on enter key #985

Merged

Conversation

jongsue
Copy link
Contributor

@jongsue jongsue commented Feb 27, 2019

fixes #866

I found the line that makes this bug

I tried to make the test case for this, but still failed it
even though it seems working fine
previous-popup-close

Popup's updateOutsideClickSubscription method has setTimeout
the test case runs before setTimeout callback

I hope someone can help me to make the test case pass

@layershifter
Copy link
Member

Actually, we should use EventStack now to handle this, but it's not fully correct. I have a prototype that solves this issue (https://codesandbox.io/s/9yw3vq6774) and will be implemented in #949.

For now going to push changes to get it working.

@codecov
Copy link

codecov bot commented Feb 28, 2019

Codecov Report

Merging #985 into master will increase coverage by 0.11%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #985      +/-   ##
==========================================
+ Coverage   81.15%   81.26%   +0.11%     
==========================================
  Files         673      671       -2     
  Lines        8654     8633      -21     
  Branches     1528     1524       -4     
==========================================
- Hits         7023     7016       -7     
+ Misses       1616     1602      -14     
  Partials       15       15
Impacted Files Coverage Δ
packages/react/src/components/Popup/Popup.tsx 74.5% <83.33%> (+1.82%) ⬆️
...ct/src/components/Dropdown/DropdownSearchInput.tsx 48.14% <0%> (-1.86%) ⬇️
...ages/react/test/specs/commonTests/isConformant.tsx 93.36% <0%> (-0.19%) ⬇️
...ackages/react/src/components/Dropdown/Dropdown.tsx 39.68% <0%> (ø) ⬆️
.../src/themes/teams/components/Icon/iconVariables.ts 100% <0%> (ø) ⬆️
packages/react/src/components/Icon/Icon.tsx 83.33% <0%> (ø) ⬆️
packages/react/src/components/Ref/RefFindNode.tsx 100% <0%> (ø) ⬆️
packages/react/test/utils/index.ts 100% <0%> (ø) ⬆️
...sibility/Behaviors/Popup/popupFocusTrapBehavior.ts 100% <0%> (ø) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4139454...bc0da61. Read the comment docs.

@layershifter layershifter added 🚀 ready for review 🧰 fix Introduces fix for broken behavior. labels Feb 28, 2019
Copy link
Collaborator

@bmdalex bmdalex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for fixing 👍 one comment
fyi @kuzhelov

@jongsue
Copy link
Contributor Author

jongsue commented Feb 28, 2019

I did extra method to reuse it as @Bugaa92 said

Thank you guys for the reviews
I've learned a lot of good points here

@kuzhelov
Copy link
Contributor

kuzhelov commented Mar 1, 2019

please, just update Fixes section of changelog before merge 👍

@layershifter layershifter merged commit 4a0cdab into microsoft:master Mar 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🧰 fix Introduces fix for broken behavior. ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Popup: previous popup is not closed on Enter key
4 participants