swup / js-plugin

A swup plugin for managing animations in JS  🎸
https://swup.js.org/plugins/js-plugin
MIT License
23 stars 5 forks source link

fix: make sure to provide a default animation #35

Closed hirasso closed 7 months ago

hirasso commented 7 months ago

Description

Also

Drive-By

Checks

Additional information

hirasso commented 7 months ago

Actually, upon reading more of the source code, we should rethink what we are doing here. createAnimationPromise actually expects an animation of null. But it never receives one in my tests with only non-matching animations

Happy to discuss in a call.

daun commented 7 months ago

I think this line will make it always pick the first animation if there's only one and it doesn't match. The whole plugin is a bit of a black box to me, admittedly...

hirasso commented 7 months ago

I'd actually like to make the matching simpler. Return the first perfect match, otherwise return the default animation.

Would be a breaking change, but well worth it IMO. What do you think?

hirasso commented 7 months ago

That's also how fragment-plugin does the route matching. Would be great to unify this between the various route plugins.

daun commented 7 months ago

What's a perfect match? I assume From and To urls both match?

The original concept of the JS plugin (I think) was to provide fallback animations by using a rating system that returns a match also for partial matches but prefers what I think you mean by perfect match. I can't say I've ever used or needed that, so I find it hard to have an opinion one way or the other here, but we're definitely removing functionality in that case.

hirasso commented 7 months ago

What's a perfect match? I assume From and To urls both match?

Yes that's what I mean

We would definitely be removing a bit of functionality if we remove partial matches. But how exactly would it benefit to animate a partial match? Wouldn't that most likely just break or by the least lead to a very brittle setup? I'd prefer a straight-forward matching logic. To be honest the partial matching makes me a bit anxious while defining my animations 😅

@gmrchk, can you remember if you ever used the partial matching behavior of JS Plugin?

daun commented 7 months ago

Haha, yeah, partial matches make me super anxious as well! Defining a fallback and enforcing strict matches makes sense to me. I was just trying to make sure we're not ripping out a feature just because the two of us don't see the value in it 🤠

The only good example scenario I can come up with is a homepage vs. any-other-page animation, in which case one would need a */ route and a reverse /* route. As long as there's wildcards, this should be doable with strict matches, right? So we're not really taking anything away, we're just enforcing verbose route definitions. Boilerplate for clarity. Sounds good to me.

daun commented 7 months ago

@hirasso Still want to merge this? Or rather switch to exact matches and release as breaking?

hirasso commented 7 months ago

Yeah... actually, I have backed away from using js plugin in my current project. Maybe I find the time in the next one! I see various other problems/chances for improvements. I'll open issues for each.