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

VT event lifecycle docs #5425

Merged
merged 17 commits into from
Nov 22, 2023
Merged

VT event lifecycle docs #5425

merged 17 commits into from
Nov 22, 2023

Conversation

matthewp
Copy link
Contributor

Description (required)

Documents the new lifecycle events from: withastro/astro#9090

Related issues & labels (optional)

For Astro version: 3.6. See astro PR #9090.

Copy link

vercel bot commented Nov 15, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
docs ✅ Ready (Inspect) Visit Preview Nov 22, 2023 2:21pm

@matthewp
Copy link
Contributor Author

I could see the following changes made:

  • Make the sections which explain the events purely about what the event does, when it occurs, etc. and not about use-cases.
  • Move use-cases to a separate headings. I like this idea because it's easier to find how to achieve something vs. finding which event lists the use-case.

@sarah11918 sarah11918 added add new content Document something that is not in docs. May require testing, confirmation, or affect other pages. merge-on-release Don't merge this before the feature is released! (MQ=approved but WAIT for feature release!) minor-release For the next minor release; in the milestone, "merge queue" when approved by Sarah! labels Nov 15, 2023

```astro
As the `astro:after-swap` event occurs immediately before the view transition ends, it's a good time to stop any loading indicators that might be running.
Copy link
Member

Choose a reason for hiding this comment

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

"occurs immediately before the view transition ends" could be misleading: Surely it is near the end of the navigation, but "astro:after-swap" takes place immediately before the start of the animation (for the simulation immediately before the start of the "new" part). In any case, the loading indicator is already recorded on the screenshot and will not disappear so quickly, depending on the animation. A better place to remove it would be astro:after-preparation because the photo has not yet been taken :)

Copy link
Member

Choose a reason for hiding this comment

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

I'm hearing two different comments here:

  1. "immediately before the view transition ends" isn't exactly right. What's a better, concise, way to describe it?
  2. Spinner has come back to haunt us. This now is the only real example (that's not, it's still safe to do this) example for this section. Can you suggest (code sample even better, but just in words like above is fine) what a good thing to happen here is? So that this section can have a proper example of its own that makes sense and is a realistic use case? Note: it's also OK to say that "there's very little that might target this event specifically, but it does provide you..."

@sarah11918
Copy link
Member

@matthewp @martrapp I took a pass at this, and added a bit more intro content to the section. It was really well done! I would be happy with this releasing with the new features, and as we have time adding more of Martin's use-cases listed in PR 9090... either in time for launch or otherwise.

I will call out:

  • astro:after-swap was already written for docs, and was previously the only point at which state was passed, so I tried to update more like to indicate that this is the "last responsible moment" for passing along state, and instead focus on the end of the view transition. I would ask you in particular to look at this and see whether you'd describe this differently now.
  • I'm not entirely sure I've nailed the difference between after-preparation and before-swap. Only one of them talked about being inside/outside of the view transition, so I made it clearer that one is before and one is inside. And I'm inferring that the difference is that before you have access to the old page only, not the new one, and inside the view transition you have access to BOTH DOMs so you can swap. I just want to make sure that that is correct and that you think that comes across. We haven't really said what "being inside the view transition" means explicitly, but I think that's the connection.

Otherwise, please read for both accuracy but also nuance! We have lots of time to get this exactly right, not just sort of right.

navigation process, not phase! phases are smaller pieces!
@matthewp
Copy link
Contributor Author

@sarah11918 Yeah, that different makes sense. Another thing that @martrapp and I were talking about, is that the before- events are points in which you can modify what is about to happen, whereas the after- events are notifications of what just happened. I'm not sure if that's something we should mention or not.

Doing another pass myself now.

src/content/docs/en/guides/view-transitions.mdx Outdated Show resolved Hide resolved
src/content/docs/en/guides/view-transitions.mdx Outdated Show resolved Hide resolved
src/content/docs/en/guides/view-transitions.mdx Outdated Show resolved Hide resolved
src/content/docs/en/guides/view-transitions.mdx Outdated Show resolved Hide resolved
matthewp and others added 5 commits November 20, 2023 17:21
Co-authored-by: Martin Trapp <94928215+martrapp@users.noreply.github.com>
Co-authored-by: Martin Trapp <94928215+martrapp@users.noreply.github.com>
Co-authored-by: Martin Trapp <94928215+martrapp@users.noreply.github.com>
Co-authored-by: Martin Trapp <94928215+martrapp@users.noreply.github.com>
Co-authored-by: Martin Trapp <94928215+martrapp@users.noreply.github.com>
@martrapp
Copy link
Member

martrapp commented Nov 21, 2023

As for the question of relevant examples for each event, you might want to check out the new examples at https://events-3bg.pages.dev/.

For the document we could do this:

  • before-preparation + loader() replacement: (simplified) image prefetch example from the demo page
  • after-preparation: existing example (loading indicator)
  • before-swap: existing example (darkMode)
  • before-swap + swap() replacement: existing example (diff)
  • after-swap: scroll to top example from the demo page
  • page-load: existing example

What do you think? @matthewp, @sarah11918

@sarah11918
Copy link
Member

@martrapp Did you make that page?? That's lovely! I would suggest we link to it, rather than reproduce it. Many of our docs pages link to "community resources" and then it's less for our translators to translate, less of a maintenance burden on docs if things update. it also encourages community members to produce content which scales more than us doing everything ourselves. 😄

What would you think about that?

@martrapp
Copy link
Member

What would you think about that?

That would be great! (Once the events are published and people can download and run the examples with a current Astro version)

But we can definitely borrow for the missing after-swap example :-)

@sarah11918
Copy link
Member

@martrapp I would love it if you could add the after-swap event example here, and then I think this would be good to go!

Co-authored-by: Martin Trapp <94928215+martrapp@users.noreply.github.com>
@sarah11918
Copy link
Member

OK! I'm happy with the content/structure of this page!

@matthewp @martrapp - happy for you to do any final review here. Otherwise, Team Docs will just handle any last minute polishing as we notice in the morning! Thank you both for your contribution for this one! An exciting feature we're happy to get into docs! 🙌

Copy link
Member

@martrapp martrapp left a comment

Choose a reason for hiding this comment

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

Layout error due to wrong quotation marks, but otherwise I'm very happy with this section!

Co-authored-by: Martin Trapp <94928215+martrapp@users.noreply.github.com>
@sarah11918 sarah11918 merged commit 5d25b87 into main Nov 22, 2023
7 checks passed
@sarah11918 sarah11918 deleted the vt-events branch November 22, 2023 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add new content Document something that is not in docs. May require testing, confirmation, or affect other pages. merge-on-release Don't merge this before the feature is released! (MQ=approved but WAIT for feature release!) minor-release For the next minor release; in the milestone, "merge queue" when approved by Sarah!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants