-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Add view transitions theme support in abstracted way with sensible defaults #8370
base: trunk
Are you sure you want to change the base?
Add view transitions theme support in abstracted way with sensible defaults #8370
Conversation
…lt for block themes.
…lassic default themes.
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
…more sense since the same view transition name can be used on multiple elements.
src/wp-includes/view-transitions.js
Outdated
const isMainSlide = transitionType === 'forwards' || transitionType === 'backwards'; | ||
let foundMainElement = false; | ||
return [ | ||
...Object.entries( config.globalTransitionNames || {} ).map( ( [ selector, name ] ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some inline comments above each of the return elements would be nice, took a bit to understand what the code was doing exactly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added docs in 220d1b8
src/wp-includes/view-transitions.js
Outdated
]; | ||
}; | ||
|
||
const setTemporaryViewTransitionNames = async ( entries, vtPromise ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this function also clears the transition names after the transitions complete, right? inline doc would be nice here as well :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added docs for all the functions in 220d1b8
❤️ Amazing work @felixarntz 🎉 I love how you managed to create such a simple way for themes to opt in to View Transitions, and haw you handled the complex bits automatically (especially figuring out if the transition is moving "forwards" or "backwards"). Also, I completely forgot about page breaks, neat that it already supports them. Overall this looks really good, my main feedback would to request a bit more inline documentation, especially for the obtuse JavaScript bits. It might also be nice to capture how these transitions look across a subset of core themes and include brief screencasts of them in action, or maybe some Playground blueprints that were preset to use them including sample content. This also looks like something we might be able to leverage eventually for the Query Loop Block, eg navigating from a post list loop to a single post. Have you already considered how that could work? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested with Twenty Twenty-One and it felt magical.
Screen.recording.2025-03-03.12.57.25.webm
src/wp-includes/view-transitions.js
Outdated
let oldPageMatches = oldPathname.match( /\/page\/(\d+)\/?$/ ); | ||
let newPageMatches = newPathname.match( /\/page\/(\d+)\/?$/ ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically the /page/
can be overridden by setting it to something else in WP_Rewrite::$pagination_base
. So ideally this would be exported from PHP as part of the config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related, I was thinking last week about whether we should even use the URLs for this or rely on classes on body
. Almost all relevant WordPress query parameters (including those for pagination) have body
classes we could use. That would make the implementation more flexible and also work on sites without pretty permalinks.
Part of the JS in this PR already used body
classes, so maybe we should use that approach more holistically?
Co-authored-by: Weston Ruter <[email protected]>
Fair point, I'll add more soon. As I'm still experimenting at this point, I didn't want to invest too much time documenting just yet, but eventually there should definitely be better docs.
Great idea! Let's work on this once the PR is in a place where we think the feature set is worth moving forward with. |
Co-authored-by: Adam Silverstein <[email protected]>
…arntz/wordpress-develop into add/view-transitions-theme-support
|
||
window.wp = window.wp || {}; | ||
window.wp.viewTransitions = {}; | ||
window.wp.viewTransitions.init = ( config ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eventually config
should have types added to it, along with the relevant type annotations throughout this file. But maybe doesn't make sense yet while still in prototype phase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do you mean that? Types as in TypeScript?
I agree it's too early now, FWIW implementing this as a Gutenberg package may be better from a technical perspective alone, since writing JavaScript in Core is subject to ancient linting requirements (GHA currently fails because of basic things like using const
in this file) 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Types as in JSDoc comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not TypeScript, but JSDoc (with maybe TypeScript in the JSDoc). For example, like we do in the PL plugin:
https://github.com/WordPress/performance/blob/trunk/plugins/performance-lab/includes/admin/plugin-activate-ajax.js
https://github.com/WordPress/performance/blob/1a7f906f96a3c6635e8a1cfdb8e36ee1ff6aa75e/plugins/optimization-detective/detect.js#L318-L339
(GHA currently fails because of basic things like using
const
in this file) 🙃
That used to be the case, but that was changed in Core-58645. So most of the JSHint complaints are actual issues:
src/js/_enqueues/wp/view-transitions.js
8 | if ( ! window.navigation || ! 'CSSViewTransitionRule' in window ) {
^ Confusing use of '!'.
25 | ? Object.entries( config.postTransitionNames || {} ).map( ( [ selector, name ] ) => {
^ Misleading line break before '?'; readers may interpret this as an expression boundary.
34 | const setTemporaryViewTransitionNames = async ( entries, vtPromise ) => {
^ 'async functions' is only available in ES8 (use 'esversion: 8').
44 | for ( const [ element, _ ] of entries ) {
^ '_' is defined but never used.
The third issue is due to:
Line 6 in 4b2c7c0
"esversion": 6, |
We should update that to 8
now. Maybe apply that change here while waiting for it to be fixed in core. I've opened Core-63077 to address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Committed to trunk
in r59963.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added docs for this function (and others) in 220d1b8.
Co-authored-by: Weston Ruter <[email protected]>
Co-authored-by: Weston Ruter <[email protected]>
While I'm not a designer, I think it's unfortunate that the index template in Twenty Twenty-Five puts the post title after the featured image: Screen.recording.2025-03-12.14.49.49.webmWhereas on the singular template, the index template has the post title before the featured image. If the index template were made consistent, this would make the transition better IMO: Screen.recording.2025-03-12.14.50.42.webm |
This PR explores adding cross-document view transitions to WordPress Core. These enable smooth transitions between URL navigations and thus can improve user experience.
Learn more about cross-document view transitions
For browser support, see https://caniuse.com/mdn-css_at-rules_view-transition
High-level approach
article.post
element or, sometimes on the singular post URL it's not wrapped specifically, but this we can detect viabody.single
selector..wp-block-post.post
element..post
element on the page that contains ana
link pointing to that same URL. That way we can achieve a nice effect where relevant post elements (in this PR so far the post title and featured image) shift to their location within the other URL.What's supported?
API
add_theme_support( 'view-transitions' )
.Transition behavior
/%year%/%monthnum%/%day%/%postname%/
).Testing
Next steps
I tested all default themes with how they behave, and it works for almost all of them:
Obviously, more at-scale in research and testing is needed, to see how other themes (especially classic themes) handle those areas in terms of markup and whether they would also work out of the box or would require customizations. But this is a start :)
Trac ticket:
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.