withastro / roadmap

Ideas, suggestions, and formal RFC proposals for the Astro project.
290 stars 29 forks source link

View Transitions RFC #607

Closed matthewp closed 1 year ago

matthewp commented 1 year ago

Summary

Provides utilities to ease the usage of view transitions in Astro sites.

Links

andparsons commented 1 year ago

Can both transition:animate and transition:persist only exist in top level .astro files (and Astro components/layouts). Specifically for the animate, I think I’m reading this as being opt in per island, rather than something I can use within the island? So I couldn’t have a <Header /> island and have a different transition:animate for a nav and an optional breadcrumb bar?

remorses commented 1 year ago

Can you explain more why “This approach is not the best for apps”? I don’t like having to use a client router for apps uses cases, you lose the ability to fetch data in the server

How do persistent islands update between pages if props change?

matthewp commented 1 year ago

@andparsons transition:animate can be set on any element, not just islands. It can't be used within an island, no.

Keep in mind that you don't need to use these directives, you can write your own animations in plain CSS. So if you wanted to animate some elements within an island you could do so by inserting <ViewTransitions /> in your head and then writing the CSS.

Of course you might want to use the built-in animations instead of writing it yourself. Currently sharing view transition animations is tricky. It's not as simple as a CSS class.

But there is something we can probably do here. Maybe something like this (React example):

import { slide, createAnimation } from 'astro:transitions';

export default function() {
  const { attrs, styles } = createAnimation('header', slide({ duration: 300 }));

  return (
    <header {...attrs}>
      <style>{styles}</style>
    </header>
  )
}
matthewp commented 1 year ago

@remorses

Can you explain more why “This approach is not the best for apps”?

Maybe I should update that section to rephrase it. All I meant there is that it doesn't turn Astro into a SPA framework. There's still islands and separate pages. For some category of apps that are multi-pages this RFC should help. But if you are building a chat app, for example, you probably still want to use a client-side router. Does this explanation help?

How do persistent islands update between pages if props change?

That's a good question. My thought was that the old island replaces the new one and that the new props are also copied over. I should update the text to explain this. My expectation is that if props change the island will rerender with them.

niklasgrewe commented 1 year ago

Does this mean that Astro is not interested in implementing a client-side router and you have to use thirtparty libraries for that?

I can understand that Astro doesn't want to become an SPA framework, but client-side routing improves the experience of website visitors a lot. In this aspect I would have been happy if Astro would offer an optional router.

That doesn't have to mean that Astro becomes something else. From my perspective, the router would just be something optional that you can use or not.

Basically, I don't think the division between websites and apps is quite right. Why shouldn't a blog or agency website use client-side routing if it improves load times and enables seamless page transitions? Personally, I think it's a shame when you have to switch back to other frameworks in such cases, which are often too complicated, error-prone, or overkill for creating content-based websites.

matthewp commented 1 year ago

This RFC does include a client side router.

niklasgrewe commented 1 year ago

@matthewp

This RFC does include a client side router.

sorry only now i understood the connections between View Transition API + Client-side Router. I am very happy about this step. Keep up the good work 👍

Akxe commented 1 year ago

So, I made the page https://gingerarchitecture.cz/ and as soon as the view transition came out, I implemented them.

matthewp commented 1 year ago

There is 1 major thing that needs updating to the RFC before it is ready for consensus:

Currently we have 2 events:

The naming on these isn't perfect. Does anyone have opinions on different names?

Akxe commented 1 year ago

How about astro:swapped and astro:beforeswap? Plain and simple...

Or if the event is to be really easy to understand astro:beforeswaprendered

natemoo-re commented 1 year ago

Personally, I think it would be nice to align with the upcoming Navigation: navigate API and use astro:navigate. Instead of having a different before event (which couldn't really wait for anything async) could we have an intercept method on the event? Given that a ton of thought has gone into the Navigation API improving on some historical complexity/footguns of the existing History APIs, it'd be nice to not repeat those same mistakes.

matthewp commented 1 year ago

What would intercept do in this context? From my understanding, that is used to prevent navigation and do SPA stuff instead. But we aren't allowing people to prevent navigation, we are already preventing navigation ourselves. If we were to implement something like that, I would probably expect for it to be a way to prevent the transition from occurring. But I don't know of a use-case for that yet.

matthewp commented 1 year ago

Here's the idea I have so far:

What I like about these names is that they are clearly different; the old names were named similarly but are for different points in the lifecycle. I also like using transition since we use that term elsewhere to describe the act of transitioning from one page state to another.

Akxe commented 1 year ago

What do you think about the standing issues highlighted in the https://github.com/withastro/roadmap/pull/607#issuecomment-1658858619 above?

matthewp commented 1 year ago

@Akxe Hey, I did see your list, thanks for reminding me. This PR is just about the API so anything that's a bug can be talked about fixed in the main repo.

For your animation ideas, we have the ability to customize them via the JS API described in this section: https://github.com/withastro/roadmap/pull/607/files#diff-d291a93498426e7f8d119f703074a53a15a3f73af71b6b1465f114d506ddf4f3R212

We did it this way for a couple of reasons:

So you should have essentially fully control over all of the animations that way.

What do you mean by a no-op transition?

Akxe commented 1 year ago

I don't think there is a way to make the default morph animation any longer/shorter.

No-op transition would be a way to enable animation from one page to another but disable it in reverse...

matthewp commented 1 year ago

@Akxe I'll take a look at the morph API and see what we can do.

For the no-op idea, we were thinking of adding an attribute, like data-astro-no-follow that you could put on a link to prevent a transition on that link. Would that fit the need?

Akxe commented 1 year ago

@matthewp I honestly don't recall why I needed it. But I have solved in an other way...

matthewp commented 1 year ago

@Akxe ok, if you remember the need let me know and I'll try and get it encorporated.

matthewp commented 1 year ago

Talked quite a bit with people on discord and realized that the proposed event names were too vague. So trying to be more exact and going with astro:afterswap and astro:navigationsuccess. RFC is updated.

matthewp commented 1 year ago

I'm moving for a call for consensus on this RFC. View transitions has been in experimental since 2.9 and the API has only changed to support persistent islands. This will be the final comment period (3 days); if there are no objections this will be merged and the feature can move out of experimental in 3.0.

Note that this doesn't mean no features / improvements can't be added, those will continue. The RFC covers the overall shape of the API.

TheOtterlord commented 1 year ago

I only have event name nitpicking. I like astro:load and astro:transition as a pair of easily understood event names. I don't mind astro:navigationsuccess and astro:afterswap, but afterswap doesn't really explain itself too well IMO. I'd see something like astro:pageload (load) and astro:navigate (beforeload) making sense maybe. I like the overall API, directives, etc. Event names can be difficult :sweat_smile:

delucis commented 1 year ago

Not 100% sure of the answer, but I do agree with @TheOtterlord re: the event naming.

afterswap alternatives

navigationsuccess alternatives

Naming style

How important is the all lowercase name? I know it’s a pattern in native events, but it’s not very accessible in the composite cases, because a screenreader often won’t detect the word boundaries and read something garbled (it’s also harder to read). Camel, kebab, or snake case would all be preferable.

Akxe commented 1 year ago

I do agree with the comments above. I would like to add my ideas...

afterswap alternatives

navigationsuccess alternatives

Akxe commented 1 year ago

I just realised that the RFC needs a way to opt-out for individual links. I just had a case where I link to a PDF file, and the transition engine tries to make a pretty transition to it, breaking completely...

matthewp commented 1 year ago

@Akxe There's a PR up for opting out of individual links.

matthewp commented 1 year ago

re: event naming. cc @TheOtterlord @Akxe @delucis One thing to keep in mind is that naming has to make sense both for view transition navigations and for non-VT fallback. So that is one reason to avoid naming things in relation to a step like startViewTransition, these events still fire in fallback mode when there is no view transition occurring.

Another thing to keep in mind is that there will likely be new events in the future. I can see a "start transition" event that gives you the transition object so that you can await it being finished, etc. So that was the reason for moving away from transition as part of the name and to afterswap which is intended to be precise.

matthewp commented 1 year ago

I agree about navigationsuccess though, I think it should probably be changed. For a couple of reasons:

I think I agree with @TheOtterlord that a astro:pageload conveys what this event is about better. Will make this change.

matthewp commented 1 year ago

I think what I'm going to do is remove the event names from the RFC text, or make it clear that they are tentative names. RFCs are not blocked on naming bikeshedding, that can continue to happen through the PR process. So happy to discuss the exact names to be used in the PR that updates it.

Akxe commented 1 year ago

re: event naming. cc @TheOtterlord @Akxe @delucis One thing to keep in mind is that naming has to make sense both for view transition navigations and for non-VT fallback. So that is one reason to avoid naming things in relation to a step like startViewTransition, these events still fire in fallback mode when there is no view transition occurring.

Why should the names respect the fallback variant? Isn't the fallback in place to fill the time it will take other browsers to implement these features? Do you plan to support the transition of browsers back down from the implementation?

matthewp commented 1 year ago

Yeah, I mean we don't have to respect it. But it can also be confusing to people who make think that they are listening to an event that only occurs during a transition and make assumptions based on that.

Given that swapping the DOM contents doesn't really have anything to do with a transition; it happens to occur inside of a transition if there is one, is there a benefit with having transition be part of the name?

tropperstyle commented 1 year ago

We already server-render with Alpine.js for client interactions and do SPA-like navigation like this (simplified):

document.addEventListener("click", e => {
    const a = e.target.closest("a");
    if (a && a.hasAttribute("x-fetch")) {
        xFetch(a.href);
        e.preventDefault();
    }
});

async function xFetch(url: string, options?: RequestInit) {
    const response = await fetch(url, options);
    const html = await response.text();
    const parser = new DOMParser();
    const doc = parser.parseFromString(html, "text/html");
    Alpine.morph(document.documentElement, doc.documentElement);
    history.pushState({}, "", response.url);
}

If we wanted to switch to this API, we would need a way to "swap" the swap algorithm

More example algorithms: https://htmx.org/docs/#morphing

API suggestion:

<ViewTransitions swap={doc => Alpine.morph(document.documentElement, doc.documentElement)}>
delucis commented 1 year ago

naming has to make sense both for view transition navigations and for non-VT fallback

Shouldn’t these two cases be transparent/equivalent to users? I’d think that when the fallback runs, users code should behave the same. In other words, the fallback behaviour is an Astro implementation detail I don’t want to think about as an API user.

I still stand by the argument that when using these APIs “after swap” is likely not a clear concept to a user. Even if we would like people not to rely on this being “before transition” (because it could be non-VT fallback), when documenting we’ll still end up writing stuff like “to ensure something happens before running the transition use the afterswap event”.

astro:pageload conveys what this event is about better. Will make this change.

Reiterating from above that all-lowercase names like pageload are suboptimal for accessibility and camel, kebab, or snake case would be preferable.

matthewp commented 1 year ago

@tropperstyle I think we can likely accommodate custom morphing. That exact API won't work because this code needs to run in the client and the one you show is running on the server. But something else, like perhaps an event could allow you to achieve this.

matthewp commented 1 year ago

Shouldn’t these two cases be transparent/equivalent to users? I’d think that when the fallback runs, users code should behave the same. In other words, the fallback behaviour is an Astro implementation detail I don’t want to think about as an API user.

For these two events it is transparent, yes. They occur in both. But there could be some events in the future that only happen with an actual transition, such as the case where we include the transition object as part of the event. It wouldn't make sense for this event to fire in fallback browsers.

Our fallback behavior is intended to be a best-effort fallback, it's not intended to be polyfill.

I still stand by the argument that when using these APIs “after swap” is likely not a clear concept to a user. Even if we would like people not to rely on this being “before transition” (because it could be non-VT fallback), when documenting we’ll still end up writing stuff like “to ensure something happens before running the transition use the afterswap event”.

Wouldn't it be clear if we documented swap as a concept and explained what it is, how it works?

To be clear, I'm not opposed to use the word transition if others don't think it becomes confusing. I do want to make sure we are accurate however. This event does not occur before the transition. It happens in the middle of it. Looking these docs: https://developer.mozilla.org/en-US/docs/Web/API/View_Transitions_API#the_view_transition_process the event we're discussing happens in step 2. The browser has already screenshotted the old page.

Reiterating from above that all-lowercase names like pageload are suboptimal for accessibility and camel, kebab, or snake case would be preferable.

Sure, happy to bikeshed in the PR. I'll link to it here. Those other options aren't conventional, built-in events are mostly lowercase like this (with some legacy exceptions). But if there's a good reason to break that convention we can do so.

matthewp commented 1 year ago

Here's the PR: https://github.com/withastro/astro/pull/8181 we have up to a week to debate :D

matthewp commented 1 year ago

The comment period has passed with no objections, so this RFC is merged as approved. Thanks everyone!