-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
v9.0.0 #632
v9.0.0 #632
Conversation
292f3e6
to
09e1e1b
Compare
Hi @aleclarson. I'm not sure that it's a right place to ask but anyway: I test it from your branch https://github.com/aleclarson/react-spring/tree/wip-1 and then https://github.com/react-spring/react-spring/tree/v9. Could you check it please? |
@Nizarius could i see the transition code? |
@drcmda i created the repository for it: https://github.com/Nizarius/Spring_test |
It seems obvious that passing config object to the specific step should change only that step's config. |
@Nizarius That will be changed. Thanks for your input! 👍 |
@aleclarson thank you. By the way, if this change is not in priority I can make PR for it. |
@drcmda Can it be useful to update the @Nizarius I can take of it, thanks! |
you mean ongoing animations? not sure where that could ever happen. |
In response to: #632 (comment)
@Nizarius Okay, should be good to go now. 🤞 LMK if anything breaks! |
In response to: #632 (comment)
@aleclarson yeah, it works as expected now! Yesterday I also noticed some behaviour that was unexpected, but it seems like you fixed it too. If it appears again I'll let you know. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Not sure if this is the right place but as this added rc.3 thought I'd put it here in case it hasn't been noticed. https://codesandbox.io/s/issue-948-ufqf4?file=/src/index.js The sandbox you created here works fine on Also I can't pinpoint where in the code this is a problem but in Firefox 48
Assuming there is a const somewhere being initialised without a value, not sure if this doesn't support this browser as it is quite old now. Some compat testing given the above example, minimum browser versions that worked:
|
@andrewdavidcostello An updated version of that demo exists here: https://codesandbox.io/s/issue-948-ggq87?file=/src/index.js (There does seem to be a hiccup, but not the one you mentioned AFAICT.) If you ever see a demo that fails with As for the Firefox syntax error, you're probably using the |
@aleclarson Thanks for the quick reply. I swapped out the I forked the linked demo here and applied the global style from the previous one as it seemed to throw it off without it: https://codesandbox.io/s/issue-948-lbz3b?file=/src/index.js Seems to be the same unless I misunderstood you, items are not removed from the dom and quickly closing items seems to result in some popping back up? |
Not sure. What are the line/column numbers?
By default, items are not unmounted until all transitions are at rest. That's probably what you're seeing. The "popping back up" issue will be fixed in the next version. 👍 |
@aleclarson Looking in Chrome 50 gave some better output. It looks to be the use of async/await within the module. Does the cjs version depend on browser support for that? Just in case some wires are crossed on the other issue: When an item is cancelled off manually this is left in the dom Create new notification, as old notification is still in the dom it reappears All clear after allowing that to progress normally. Both items are removed from dom. |
Nope, that's compiled to use regenerator. Line and column numbers?
Yeah, I've already fixed the bug causing that. Will be released soon. |
@aleclarson Thanks a lot for your help really appreciate it, asking for the line and column numbers made me realise something was up so I did a prod build and everything is now a-ok. Well that and the fact I was importing animate into another file from the non cjs version 🤦 Have noticed on Chrome 50 this error:
Which is easy to polyfill but as the cjs versions seem to be aiming for compatibility just a heads up in case you want to change that. |
7695a80
to
b818f08
Compare
8eba6d6
to
f3a0576
Compare
3d63523
to
f676b1e
Compare
4325192
to
a15e98a
Compare
afac367
to
55a1379
Compare
11f48aa
to
efc8e56
Compare
Follow-up to #615
Changelog
Closes #335
Closes #362
Closes #383
Closes #432
Closes #436
Closes #461
Closes #535
Closes #559
Closes #571
Closes #576
Closes #591
Closes #594
Closes #613
Closes #623
Closes #624
Closes #629
Closes #633
Closes #634
Closes #637
Closes #641