Replace Twenty Twenty-One custom JS frontend code with Interactivity API #5795
Conversation
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. |
|
Some open questions:
cc @luisherranz @swissspidy @joemcgill @mukeshpanchal27 for visibility |
|
Meh: JSHint complains because we're using modern JavaScript (it's a module, duh 😂) |
| @@ -27,8 +27,15 @@ | |||
| function twenty_twenty_one_add_sub_menu_toggle( $output, $item, $depth, $args ) { | |||
| if ( 0 === $depth && in_array( 'menu-item-has-children', $item->classes, true ) ) { | |||
|
|
|||
| // Extra attributes depending on whether or not the Interactivity API is being used. | |||
There was a problem hiding this comment.
Nice side effect: avoiding onClick inline script is better for CSP
Interactivity API does not require import maps and modules, does it? I thought the import maps stuff was a separate experiment in Gutenberg. |
Looks like it does. I didn't enable any experiments, and the Interactivity API via Gutenberg is loaded as a module. In fact, there's a minor issue there, if I enqueue Overall, I certainly think using a module is a good idea though. |
There was a problem hiding this comment.
What a fantastic exploration, @felixarntz!! 😄
I think the code is great and it shows how to use the Interactivity API pretty well.
The only modification I would do is to use directives for the submenus to make them declarative, instead of manipulating the DOM manually: 762b3b
But other than that, awesome work! ❤️
|
|
||
| toggleDarkMode: () => { | ||
| state.isDarkMode = ! state.isDarkMode; | ||
| window.localStorage.setItem( 'twentytwentyoneDarkMode', state.isDarkMode ? 'yes' : 'no' ); |
There was a problem hiding this comment.
Not that this is wrong, but noting here that this can also be done reactively, which is a bit safer in cases where state.isDarkMode would be modified from a few different places:
data-wp-watch="callbacks.storeDarkMode"callbacks: {
storeDarkMode: () => {
// This callback is triggered each time `state.isDarkMode` changes, no matter from where.
window.localStorage.setItem( 'twentytwentyoneDarkMode', state.isDarkMode ? 'yes' : 'no' );
}
}There was a problem hiding this comment.
@luisherranz Follow up question: Wouldn't your suggestion call window.localStorage.setItem any time that any of the store's state properties are updated? If not, how does it not only to look for state.isDarkMode changes?
There was a problem hiding this comment.
The callback only runs when the state.isDarkMode value changes because it's only subscribed to the state.isDarkMode signal.
That's simply how signals work: data-wp-watch is a computation and when you access a property (state.isDarkMode), before returning its value, it checks if it's inside a computation. If it is, it adds the computation to its internal graph of dependencies. When state.isDarkMode is updated, it invalidates all its computations, and the computation callbacks run again, updating their values/DOM.
In the Interactivity API, the interface is transparent (transparent reactive programming) because we are using Proxies on top of state and context to track their access/assignments.
By the way, terms are a bit vague in the world of signals. Computations are sometimes called effects, reactions or observers, and signals are sometimes called observables or reactive variables. Oh, and now with Svelte 5, they are also called runes 😆 .
There was a problem hiding this comment.
@luisherranz Thanks! Updated in b3af9e0 (please ignore the whitespace changes)
Another follow up question: I have now two data-wp-watch callbacks for the same state property on the same HTML element. I think that makes sense, given that they serve different purposes, but I wanted to check whether there's any downside to this approach compared to putting the logic for both into the same callback, e.g. performance-wise.
There was a problem hiding this comment.
@luisherranz Actually, on second thought, I'm not sure that this change makes sense - from a functionality perspective:
- The
isDarkModevalue should only be cached inlocalStorageif it was modified by the user. - With the change in the new commit, I don't think that's the case any longer. Because it reacts to any
isDarkModechanges, I believe it now also caches the initial value that comes purely from the browser detection. The original code only cached the value when invoked via the toggle, which is the intention. - Let me know if this makes sense. Do you have any suggestions how to achieve that functionality in a way that still uses
data-wp-watch? Or best to go back to my previous code where it happens together with the toggling? 🤔
There was a problem hiding this comment.
Can the user "unselect" the color preference to go back to prefers-color-scheme? So toggle between light/dark/prefers-color-scheme?
If so, I'd check the value of the setting in the watch callback and use removeItem() when the user has selected prefers-color-scheme, which should also be the initial value and therefore nothing should be saved on page load.
Would that make sense?
There was a problem hiding this comment.
@luisherranz That isn't possible. As soon as the user makes a choice in that UI, it's "permanent".
There was a problem hiding this comment.
Ok. So, even though the user can't go back to system once they select light/dark, there are three possible values. So what I would do is to replace state.isDarkMode = true | false, which only stores two values, with state.colorMode = 'system' | 'light' | 'dark' to fully represent all the possibilities.
The default value should be system (you can define it in PHP using wp_interactivity_state or in JS directly in the store definition). Then, I'd modify the watch callback to do something like this:
callbacks: {
storeDarkMode: () => {
if ( state.colorMode !== 'system' ) {
window.localStorage.setItem(
'twentytwentyoneDarkMode',
state.colorMode === 'dark' ? 'yes' : 'no'
);
}
}
}I'm assuming you need to keep twentytwentyoneDarkMode for backward compatibility but if not you could use twentytwentyoneColorMode and store light/dark.
Does that make sense?
There was a problem hiding this comment.
@luisherranz That's a good idea, however with one modification necessary: We can't only store system, we would need to still know whether the system's value is light or dark. So we could use something like system-light and system-dark, but at that point I feel like we're overloading that property.
I went with two separate properties: isDarkMode remains as before, while a separate boolean property isDarkModeManuallyOverwritten (yes, very cumbersome name 😆) stores whether it's the system default or manually overwritten.
See 4a35f3f
There was a problem hiding this comment.
Ok. But you should derive isDarkMode. Remember that all the state should have only one source of truth, so all the state that can be derived, should be derived.
Something like this:
state: {
colorPreference: window.localStorage.getItem( 'twentytwentyoneColorPreference' ) || "system",
get isDarkMode() {
if ( state.colorPreference === "system" )
return window.matchMedia('(prefers-color-scheme: dark)').matches ? "dark" : "light";
return state.colorPreference;
}
},
actions: {
toggleDarkMode() {
state.colorPreference = state.colorPreference !== "dark" ? "dark" : "light";
}
},
callbacks: {
storeColorPreference() {
if ( state.colorPreference !== 'system' ) {
window.localStorage.setItem(
'twentytwentyoneColorPreference',
state.colorPreference
);
}
}
}|
|
||
| state.isDarkMode = isDarkMode; | ||
|
|
||
| // The following may be needed here since we can't use `data-wp-on--scroll`? |
There was a problem hiding this comment.
data-wp-on--scroll works fine, but it's scoped to the element that has the directive:
<div data-wp-on--scroll="callbacks.onScroll">...</div>// element is the <div>
element.addEventListener("scroll", callbacks.onScroll);But here, you want to use the global scroll event: window.addEventListener( 'scroll', checkScroll ).
We don't have any special syntax for directives attached to global events yet, so for now it's fine to use data-wp-init. Once we see how common is this, maybe we can come up with something more specific, like data-wp-global-event--scroll or something like that.
What do you think? 🙂
There was a problem hiding this comment.
I think having a more declarative way via attribute to add listeners for global / window events would be great. Maybe data-wp-on-global--{event} or data-wp-on-window--{event}? This could then work for any window events, like scroll, resize etc.
There was a problem hiding this comment.
Makes sense. Would you mind opening a new discussion to talk about it? 🙂
There was a problem hiding this comment.
I've opened a related discussion:
There was a problem hiding this comment.
@luisherranz I've updated this PR to use the new data-wp-on-window and data-wp-on-document in 44e4977, reducing the amount of custom JS further. 🎉
cc @c4rl0sbr4v0
| }, | ||
|
|
||
| refreshHtmlElementDarkMode: () => { | ||
| // This hack may be needed since the HTML element cannot be controlled with the API attributes? |
There was a problem hiding this comment.
<html> is not supported as an interactivity target, so this is the exception where mutating the DOM manually is correct.
Oh, and doing so reactively with a data-wp-watch like you're doing here is the best possible way to do so because it will always be in sync, no matter where state.isDarkMode is changed 🙂
| data-wp-interactive='{"namespace": "twentytwentyone"}' | ||
| data-wp-class--primary-navigation-open="state.isPrimaryMenuOpen" | ||
| data-wp-class--lock-scrolling="state.isPrimaryMenuOpen" | ||
| data-wp-class--is-dark-theme="state.isDarkMode" | ||
| data-wp-init--iframes="callbacks.updateWindowWidthOnResize" | ||
| data-wp-watch--iframes="callbacks.makeIframesResponsive" | ||
| data-wp-init--darkmode="callbacks.initDarkMode" | ||
| data-wp-watch--darkmode="callbacks.refreshHtmlElementDarkMode" |
There was a problem hiding this comment.
Using directives in <body> is fine 🙂
There was a problem hiding this comment.
My concern is that the <body> element isn't in any way "owned" by a specific plugin or theme. Normally, regions can be assumed to be subject to a specific plugin or theme (e.g. a block, or a specific HTML container element), but <body> may be manipulated by anyone. For instance, any plugin could add a dynamic body class, in which case it would need to use directives on the body.
How could that work? Wouldn't there be conflicts between namespaces? That's where my concern stems from.
There was a problem hiding this comment.
I guess it's fine for the theme to set it to its own namespace. After all, the namespace option in data-wp-interactive is just a shorter way to say "the default namespace of the directives of this element and its children is X (until you find another data-wp-interactive that overrides the value)".
Other plugins that want to use directives in the <body> element can do so using the non-default syntax to avoid conflicts: data-wp-class--my-plugin-class="myPlugin::state.someClass".
What do you think? Would that be enough?
There was a problem hiding this comment.
@luisherranz Thanks, I didn't know it was even possible to put namespaces into the attribute values as a prefix. Are both of these methods fully supported? For example, could I choose to not put data-wp-interactive anywhere but instead prefix every single action/callback/state reference with the namespace (e.g. data-wp-class--primary-navigation-open="twentytwentyone::state.isPrimaryMenuOpen")?
While generally that would be cumbersome, for something like <body> which by definition isn't "owned" by any particular plugin or theme I'm thinking it may be more appropriate to not use a default namespace.
There was a problem hiding this comment.
data-wp-interactive is still required for hydration. It can be empty if you don't want to set a default namespace, though.
By the way, you can use a single string now: data-wp-interactive="twentytwentyone" 🙂
There was a problem hiding this comment.
That seems to be a quite significant limitation to me. Is there a reasonable way this could be fixed in the Interactivity API?
There was a problem hiding this comment.
I'll check it 👀
Directives using a different namespace should work, as there is other namespaces compatibility.
But I will add a test just to be sure that SSR is working as expected.
There was a problem hiding this comment.
This PR would fix it, but I would love some feedback from @DAreRodz. As is setting a default 'WP' namespace that every developer could read and use.
cc: @felixarntz @gziolo
There was a problem hiding this comment.
I don't have enough context to help. What implications the namespace have on the client? The fix proposed only changes the default value from null to WP. Does it mean that devs will have to explicitly use WP as a namespace when interacting with stores?
There was a problem hiding this comment.
No, stores should infer the namespace. But all interactive chunks created with just data-wp-interactive would use that default one, being accesible.
It would create a "rule" and, as @luisherranz mentions, could create unexpected bugs as would have a different behavior than the client.
Let's close that one and work on a better solution.
src/wp-content/themes/twentytwentyone/assets/js/interactivity.js
Outdated
Show resolved
Hide resolved
By the way, making this part declarative brings the Interactivity API JS lines down to 189, vs the 314 of the vanilla JS approach, a reduction of 40% in the amount of code that the developer needs to write. |
This PR uses the Interactivity API to replace the custom frontend JS code for the Twenty Twenty-One theme. More concretely:
interactivity.jsmodule).primary-navigation.js,responsive-embeds.js,dark-mode-toggler.js) as it replaces those functionalities.One important consideration: Since the server-side portion of the Interactivity API does not process this content automatically (unlike blocks, which are processed server-side), it is important to hydrate any dynamic attributes to have the correct initial state. For example, if you add
data-wp-bind--aria-expanded="state.something"to a tag, you should also set the actualaria-expanded="true|false"attribute depending on what the initial value ofstate.somethingis expected to be.This is an experimental PR, primarily as a proof of concept and to get a better idea of how the API works, and what should or should not be done with it.
To test it, you need to have the Gutenberg plugin activated. You should then see a
script[type="module"]pointing to the new Twenty Twenty-Oneinteractivity.jsfile in the<head>.A few observations:
Unrelated: While working on this, I discovered and reported https://core.trac.wordpress.org/ticket/60111 which I think is a bug.
Trac ticket: https://core.trac.wordpress.org/ticket/61027
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.