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

v9.0.0 #632

Closed
wants to merge 1,289 commits into from
Closed

v9.0.0 #632

wants to merge 1,289 commits into from

Conversation

aleclarson
Copy link
Contributor

@aleclarson aleclarson commented Apr 10, 2019

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

@Nizarius
Copy link
Contributor

Hi @aleclarson. I'm not sure that it's a right place to ask but anyway:
You said that #629 was implemented in #615 so I went to test it as I need this feature as soon as possible.

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.
If i pass config object with
{ duration: 0 }
to one of the next function in useTransition (update lifecycle), it overrides duration for all the other phases and all the new update lifecycles.

Could you check it please?

@aleclarson
Copy link
Contributor Author

aleclarson commented Apr 14, 2019

@Nizarius Currently, if you provide a config, it will be saved for future animations. In the case of duration, that behavior is probably a bad idea. WDYT @drcmda?

@drcmda
Copy link
Member

drcmda commented Apr 14, 2019

@Nizarius could i see the transition code?

@Nizarius
Copy link
Contributor

@drcmda i created the repository for it: https://github.com/Nizarius/Spring_test

@Nizarius
Copy link
Contributor

It seems obvious that passing config object to the specific step should change only that step's config.
Besides, it changes config not only for the specific execution (to the last step on that lifecycle) but for all updates in the future.

@aleclarson
Copy link
Contributor Author

@Nizarius That will be changed. Thanks for your input! 👍

@Nizarius
Copy link
Contributor

@aleclarson thank you. By the way, if this change is not in priority I can make PR for it.
It's really crucial for me but I don't want to do double work.

@aleclarson
Copy link
Contributor Author

aleclarson commented Apr 14, 2019

@drcmda Can it be useful to update the config of existing animations? If we change this behavior, the only workaround is accessing the controllers through the ref API and manually editing the animations of each controller. But if it's rarely useful, we should do what @Nizarius wants.

@Nizarius I can take of it, thanks!

@drcmda
Copy link
Member

drcmda commented Apr 14, 2019

you mean ongoing animations? not sure where that could ever happen.

aleclarson added a commit that referenced this pull request Apr 14, 2019
@pmndrs pmndrs deleted a comment from drcmda Apr 14, 2019
@pmndrs pmndrs deleted a comment from drcmda Apr 14, 2019
@pmndrs pmndrs deleted a comment from drcmda Apr 14, 2019
@aleclarson
Copy link
Contributor Author

aleclarson commented Apr 14, 2019

@Nizarius Okay, should be good to go now. 🤞 LMK if anything breaks!

@Nizarius
Copy link
Contributor

@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.
Thank you!

@Nizarius

This comment has been minimized.

@Nizarius

This comment has been minimized.

@aleclarson
Copy link
Contributor Author

@Nizarius You don't understand when the update lifecycle triggers. Please move your comments to here: #621

@andrewdavidcostello
Copy link

andrewdavidcostello commented May 13, 2020

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 9.0.0-canary.809.5.f01ecc2 but on rc3 when manually removing items they are no longer removed, in fact if you keep clicking seems to exhibit some strange behaviour.

Also I can't pinpoint where in the code this is a problem but in Firefox 48

syntax error missing = in const declaration

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:

Firefox 52
Chrome 55
Safari 11
Edge 79
Yandex 17.4

@aleclarson
Copy link
Contributor Author

aleclarson commented May 13, 2020

@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 9.0.0-rc.3, be sure to check the v9 branch of the react-spring-examples repo before reaching out.

As for the Firefox syntax error, you're probably using the .js bundle, which has modern syntax in it. You need the .cjs.js bundle instead. More info here: #601

@andrewdavidcostello
Copy link

@aleclarson Thanks for the quick reply. I swapped out the .js with .cjs.js which fixed that Firefox error but now gives the error SyntaxError: missing ; before statement. Any thoughts?

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?

@aleclarson
Copy link
Contributor Author

aleclarson commented May 13, 2020

now gives the error SyntaxError: missing ; before statement. Any thoughts?

Not sure. What are the line/column numbers?

items are not removed from the dom and quickly closing items seems to result in some popping back up?

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. 👍

@andrewdavidcostello
Copy link

@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
Screenshot 2020-05-13 at 15 49 49

Create new notification, as old notification is still in the dom it reappears
Screenshot 2020-05-13 at 15 49 54

All clear after allowing that to progress normally. Both items are removed from dom.
Screenshot 2020-05-13 at 15 49 24

@aleclarson
Copy link
Contributor Author

It looks to be the use of async/await within the module. Does the cjs version depend on browser support for that?

Nope, that's compiled to use regenerator. Line and column numbers?

Create new notification, as old notification is still in the dom it reappears

Yeah, I've already fixed the bug causing that. Will be released soon.

@andrewdavidcostello
Copy link

@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:

Object.values is not a function

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment