Skip to content

Fix motion popups sometimes getting stuck (#157) #158

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

Merged
merged 3 commits into from
Jan 28, 2020

Conversation

mogzol
Copy link
Contributor

@mogzol mogzol commented Dec 12, 2019

See issue #157

If the popup transitioned from visible to invisible before the status reached 'motion', the popup would get stuck in a visible state. This changes the code to immediately set status to 'stable'
during a transition from visible to invisible if the status is something before 'motion'.

Also fixes an issue where an animation frame could change the status to something invalid after a transition from visible to invisible.

If the popup transitioned from visible to invisible before the
status reached 'motion', the popup would get stuck in a visible
state. This changes the code to immediately set status to 'stable'
during a transition from visible to invisible if the status is
something before 'motion'.

Also fixes an issue where an animation frame could change the status
to something invalid after a transition from visible to invisible.
@vercel
Copy link

vercel bot commented Dec 12, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/react-component/trigger/re348m8kz
✅ Preview: https://trigger-git-fork-mogzol-animation-state-fix.react-component.now.sh

@codecov
Copy link

codecov bot commented Dec 12, 2019

Codecov Report

Merging #158 into master will increase coverage by 1.51%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #158      +/-   ##
==========================================
+ Coverage    83.9%   85.42%   +1.51%     
==========================================
  Files           7        7              
  Lines         466      494      +28     
  Branches      124      138      +14     
==========================================
+ Hits          391      422      +31     
+ Misses         75       72       -3
Impacted Files Coverage Δ
src/Popup.tsx 90.62% <100%> (+0.38%) ⬆️
src/mock.tsx 100% <0%> (ø) ⬆️
src/index.tsx 81.95% <0%> (+2.21%) ⬆️

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 c7811aa...fca787b. Read the comment docs.

@zombieJ
Copy link
Member

zombieJ commented Jan 13, 2020

Could you pls to help create related test case on this?

@mogzol
Copy link
Contributor Author

mogzol commented Jan 16, 2020

@zombieJ I've added some tests for the changes

@@ -136,6 +143,9 @@ class Popup extends Component<PopupProps, PopupState> {
const { status } = this.state;
const { getRootDomNode, visible, stretch } = this.props;

// If there is a pending state update, cancel it, a new one will be set if necessary
this.cancelFrameState();
Copy link
Member

Choose a reason for hiding this comment

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

This will block status go next if parent node re-render.

Copy link
Contributor Author

@mogzol mogzol Jan 23, 2020

Choose a reason for hiding this comment

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

But the code after this line will call setStateOnNextFrame again if necessary, so status will still get changed.

Without this, then if visible is changed to false before the next animation frame happens, then when the frame does happen it may change the status away from stable and cause the popup to appear when it shouldn't.

@zombieJ zombieJ merged commit 2783418 into react-component:master Jan 28, 2020
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.

2 participants