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

Finalize View Transition event names #8181

Merged
merged 2 commits into from
Aug 24, 2023
Merged

Finalize View Transition event names #8181

merged 2 commits into from
Aug 24, 2023

Conversation

matthewp
Copy link
Contributor

@matthewp matthewp commented Aug 21, 2023

Do not merge! This is a draft as we biekshed these names. Will undraft once consensus is reached.

Changes

  • From the RFC: View Transitions RFC roadmap#607
  • Renames the astro:load to astro:page-load
    • astro:load is already taken.
  • Renames astro:beforeload to astro:after-swap.
    • The event fires immediate after the DOM contents are swapped, so you can update the DOM to prevent FOUC
      • Most common use-case is dark mode.

Testing

  • Test updated

Docs

  • TBD, after the final naming decision is made.

Points of contention

Based on other discussions from the RFC, I think these are the major points of contention to decide:

  1. The afterswap name doesn't mention the transition. Some think it should.
  2. The names are lowercased without dashes or camelcasing. The reasoning is that this follows the naming convention of built-in events.

@changeset-bot
Copy link

changeset-bot bot commented Aug 21, 2023

🦋 Changeset detected

Latest commit: 51861ef

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@matthewp matthewp marked this pull request as draft August 21, 2023 20:56
@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) semver: minor Change triggers a `minor` release labels Aug 21, 2023
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

@matthewp
Copy link
Contributor Author

My major concern with using transition as part of the name for what is afterswap in this PR is that a transition is not just 1 step. From the MDN docs there are 6 steps. Additionally within our own code there are more steps. We might want to add events in the future for some of these steps and I want to avoid vague names that might clash with future event names.

@matthewp
Copy link
Contributor Author

For reference, here are other similar libraries and their event names:

  • htmx uses htmx:beforeSwap and htmx:afterSwap.
  • swup uses content:replace.
  • Turbo uses turbo:render (I think that's the one).

@lilnasy
Copy link
Contributor

lilnasy commented Aug 21, 2023

Turbo uses turbo:render

I think that could be misconstrued because rendering (to HTML) is still happening on the server.

@delucis
Copy link
Member

delucis commented Aug 21, 2023

  1. The afterswap name doesn't mention the transition. Some think it should.

I think I am coming around to your argument. I don’t love the naming, but if we think we’ll need more fine-grained events and other names block that, we can aim to document the technical underpinnings so that afterswap is understood. Seeing the other tools using similar names is also reassuring.

  1. The names are lowercased without dashes or camelcasing. The reasoning is that this follows the naming convention of built-in events.

We already break this convention by including a : in the event name, so I don’t think we have to adhere to it. (I’m also guessing this convention is mainly due to attribute form of <el onclick=""> etc. which won’t apply to these events either.)

Reasons the convention is bad:

  • less readable than a style where words are split up by casing differences (afterSwap) or a separator character (after-swap, after_swap)
  • less accessible, because screen readers can’t detect the word boundary and are liable to mangle the pronunciation (quick VoiceOver test: afterswap — “afters waap”; afterSwap — “after swap”; after-swap — “after swap”; after_swap — “after underscore swap”)

@matthewp
Copy link
Contributor Author

Here's some docs. Want to see if this helps with the naming: withastro/docs#4320

Going to update this PR to use dashes.

@matthewp matthewp marked this pull request as ready for review August 23, 2023 14:19
@Akxe
Copy link

Akxe commented Aug 23, 2023

I would like to list all possible events we (you) may want to fire for a single transition:

Order Short User usage Description
1. beforeUnload Use to save any component state that would be destroyed. event.preventDefault() would allow cancel the navigation/transition Before anything is done, this would be
2. transitionStart Not sure, just an FWI? It is basically at the same time, but it marks the start of animation
3. afterSwap Modify HTML before it is rendered for the first time The head is diffed, the body is swapped, and parts that should remain did remain
4. transitionEnd Only reason is, if some changes need to be done after initially rendering the final state The end of the animation
5. pageLoad Restore state to any component that may need it New styles were added, and the script already ran

Is this right?

@matthewp
Copy link
Contributor Author

Yes, that's excellent @Akxe! The intent of this PR is not to add all possible events, but just to solidify the names. I like that you have a list though. I'd probably recommend adding that to a roadmap discussion issue, and we can use that as a guide when adding new events. I could also see some table like this making its way into the docs.

@Akxe
Copy link

Akxe commented Aug 23, 2023

@matthewp My intent was not to add them, but to know what all the possible events would be. It could help us define what event would be used for what, and also it would help us name the events consistently across the whole action.

@Akxe
Copy link

Akxe commented Aug 24, 2023

I do think that the addition of beforeUnload is a good idea because if a user keeps a form partially filled, the page should be able to prevent his departure.

Also, ViewTransition API uses events ready, finished (and to some degree updateDone). I am not sure we want to use those, but it is worth considering since they would mimic the naming from the standard that Astro ViewTransition is using/mimicing.

Finally, I want to share my opinion; I think that adding astro-transition: prefix to these events might prove helpful. Even more, if Astro would add more events in future.

@matthewp
Copy link
Contributor Author

Yeah, I intend to write up a new proposal shortly that includes some new events, I agree that's probably the next one to come, and some other additions such as a JS API to trigger a navigation to occur.

Base automatically changed from next to main August 24, 2023 14:38
@matthewp matthewp merged commit a8f3577 into main Aug 24, 2023
13 checks passed
@matthewp matthewp deleted the vt-event-names branch August 24, 2023 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants