-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Replace Twenty Twenty-One custom JS frontend code with Interactivity API #5795
base: trunk
Are you sure you want to change the base?
Changes from 1 commit
3b84e7d
3e603d7
50adcfa
44e4977
0d749e3
b3af9e0
5ccb56a
4a35f3f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,206 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { store, getContext, getElement } from '@wordpress/interactivity'; | ||
|
||
function checkClass( element, className ) { | ||
if ( element.classList.contains( className ) ) { | ||
return element; | ||
} | ||
if ( element.parentElement && element.parentElement.classList.contains( className ) ) { | ||
return element.parentElement; | ||
} | ||
if ( element.parentElement.parentElement && element.parentElement.parentElement.classList.contains( className ) ) { | ||
return element.parentElement.parentElement; | ||
} | ||
return null; | ||
} | ||
|
||
const { state, actions } = store( 'twentytwentyone', { | ||
state: { | ||
isPrimaryMenuOpen: false, | ||
windowWidth: 0, | ||
prevScroll: 0, | ||
isDarkMode: false, | ||
isDarkModeTogglerHidden: false, | ||
}, | ||
actions: { | ||
togglePrimaryMenu: () => { | ||
state.isPrimaryMenuOpen = ! state.isPrimaryMenuOpen; | ||
}, | ||
|
||
openPrimaryMenu: () => { | ||
state.isPrimaryMenuOpen = true; | ||
}, | ||
|
||
closePrimaryMenu: () => { | ||
state.isPrimaryMenuOpen = false; | ||
}, | ||
|
||
toggleDarkMode: () => { | ||
state.isDarkMode = ! state.isDarkMode; | ||
window.localStorage.setItem( 'twentytwentyoneDarkMode', state.isDarkMode ? 'yes' : 'no' ); | ||
}, | ||
|
||
trapFocusInModal: ( event ) => { | ||
if ( ! state.isPrimaryMenuOpen ) { | ||
return; | ||
} | ||
|
||
const ctx = getContext(); | ||
|
||
const escKey = event.keyCode === 27; | ||
if ( escKey ) { | ||
event.preventDefault(); | ||
actions.closePrimaryMenu(); | ||
if ( ctx.firstFocusable ) { | ||
ctx.firstFocusable.focus(); | ||
} | ||
return; | ||
} | ||
|
||
const tabKey = event.keyCode === 9; | ||
const shiftKey = event.shiftKey; | ||
const activeEl = document.activeElement; // eslint-disable-line @wordpress/no-global-active-element | ||
|
||
if ( ! shiftKey && tabKey && ctx.lastFocusable === activeEl ) { | ||
event.preventDefault(); | ||
if ( ctx.firstFocusable ) { | ||
ctx.firstFocusable.focus(); | ||
} | ||
return; | ||
} | ||
|
||
if ( shiftKey && tabKey && ctx.firstFocusable === activeEl ) { | ||
event.preventDefault(); | ||
if ( ctx.lastFocusable ) { | ||
ctx.lastFocusable.focus(); | ||
} | ||
return; | ||
} | ||
|
||
// If there are no elements in the menu, don't move the focus | ||
if ( tabKey && ctx.firstFocusable === ctx.lastFocusable ) { | ||
event.preventDefault(); | ||
} | ||
}, | ||
|
||
listenToSpecialClicks: ( event ) => { | ||
const ctx = getContext(); | ||
|
||
// Check if this was a `.sub-menu-toggle` click. | ||
const subMenuToggle = checkClass( event.target, 'sub-menu-toggle' ); | ||
if ( subMenuToggle ) { | ||
if ( ctx.activeSubmenu === subMenuToggle ) { | ||
ctx.activeSubmenu = null; | ||
} else { | ||
ctx.activeSubmenu = subMenuToggle; | ||
} | ||
return; | ||
} | ||
|
||
// Otherwise, check if this was an anchor link click. | ||
if ( ! event.target.hash ) { | ||
return; | ||
} | ||
|
||
actions.closePrimaryMenu(); | ||
|
||
// Wait 550 and scroll to the anchor. | ||
setTimeout( () => { | ||
var anchor = document.getElementById( event.target.hash.slice( 1 ) ); | ||
if ( anchor ) { | ||
anchor.scrollIntoView(); | ||
} | ||
}, 550 ); | ||
}, | ||
}, | ||
callbacks: { | ||
determineFocusableElements: () => { | ||
if ( ! state.isPrimaryMenuOpen ) { | ||
return; | ||
} | ||
|
||
const ctx = getContext(); | ||
const { ref } = getElement(); | ||
const elements = ref.querySelectorAll( 'input, a, button' ); | ||
|
||
ctx.firstFocusable = elements[ 0 ]; | ||
ctx.lastFocusable = elements[ elements.length - 1 ]; | ||
}, | ||
|
||
refreshSubmenus: () => { | ||
const ctx = getContext(); | ||
const { ref } = getElement(); | ||
const elements = ref.querySelectorAll( '.sub-menu-toggle' ); | ||
elements.forEach( ( subMenuToggle ) => { | ||
if ( ctx.activeSubmenu === subMenuToggle ) { | ||
subMenuToggle.setAttribute( 'aria-expanded', 'true' ); | ||
} else { | ||
subMenuToggle.setAttribute( 'aria-expanded', 'false' ); | ||
} | ||
} ); | ||
}, | ||
|
||
makeIframesResponsive: () => { | ||
const { ref } = getElement(); | ||
|
||
ref.querySelectorAll( 'iframe' ).forEach( function( iframe ) { | ||
// Only continue if the iframe has a width & height defined. | ||
if ( iframe.width && iframe.height ) { | ||
// Calculate the proportion/ratio based on the width & height. | ||
proportion = parseFloat( iframe.width ) / parseFloat( iframe.height ); | ||
// Get the parent element's width. | ||
parentWidth = parseFloat( window.getComputedStyle( iframe.parentElement, null ).width.replace( 'px', '' ) ); | ||
// Set the max-width & height. | ||
iframe.style.maxWidth = '100%'; | ||
iframe.style.maxHeight = Math.round( parentWidth / proportion ).toString() + 'px'; | ||
} | ||
} ); | ||
}, | ||
|
||
updateWindowWidthOnResize: () => { | ||
// The following may be needed here since we can't use `data-wp-on--resize`? | ||
felixarntz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const refreshWidth = () => { | ||
state.windowWidth = window.innerWidth; | ||
} | ||
window.onresize = refreshWidth; | ||
}, | ||
|
||
initDarkMode: () => { | ||
let isDarkMode = window.matchMedia( '(prefers-color-scheme: dark)' ).matches; | ||
|
||
if ( 'yes' === window.localStorage.getItem( 'twentytwentyoneDarkMode' ) ) { | ||
isDarkMode = true; | ||
} else if ( 'no' === window.localStorage.getItem( 'twentytwentyoneDarkMode' ) ) { | ||
isDarkMode = false; | ||
} | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
<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 We don't have any special syntax for directives attached to global events yet, so for now it's fine to use What do you think? 🙂 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think having a more declarative way via attribute to add listeners for global / There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. Would you mind opening a new discussion to talk about it? 🙂 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've opened a related discussion: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @luisherranz I've updated this PR to use the new cc @c4rl0sbr4v0 |
||
const checkScroll = () => { | ||
const currentScroll = window.scrollY || document.documentElement.scrollTop; | ||
if ( | ||
currentScroll + ( window.innerHeight * 1.5 ) > document.body.clientHeight || | ||
currentScroll < state.prevScroll | ||
) { | ||
state.isDarkModeTogglerHidden = false; | ||
} else if ( currentScroll > state.prevScroll && 250 < currentScroll ) { | ||
state.isDarkModeTogglerHidden = true; | ||
} | ||
state.prevScroll = currentScroll; | ||
} | ||
window.addEventListener( 'scroll', checkScroll ); | ||
}, | ||
|
||
refreshHtmlElementDarkMode: () => { | ||
// This hack may be needed since the HTML element cannot be controlled with the API attributes? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Oh, and doing so reactively with a |
||
if ( state.isDarkMode ) { | ||
document.documentElement.classList.add( 'is-dark-theme' ); | ||
} else { | ||
document.documentElement.classList.remove( 'is-dark-theme' ); | ||
} | ||
}, | ||
}, | ||
} ); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,17 @@ | |
<?php wp_head(); ?> | ||
</head> | ||
|
||
<body <?php body_class(); ?>> | ||
<body | ||
<?php body_class(); ?> | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using directives in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My concern is that the 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess it's fine for the theme to set it to its own namespace. After all, the Other plugins that want to use directives in the What do you think? Would that be enough? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 While generally that would be cumbersome, for something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
By the way, you can use a single string now: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, stores should infer the namespace. But all interactive chunks created with just 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. |
||
> | ||
<?php wp_body_open(); ?> | ||
<div id="page" class="site"> | ||
<a class="skip-link screen-reader-text" href="#content"> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice side effect: avoiding |
||
if ( function_exists( 'gutenberg_register_module' ) ) { | ||
$extra_attr = ''; | ||
} else { | ||
$extra_attr = ' onClick="twentytwentyoneExpandSubMenu(this)"'; | ||
} | ||
|
||
// Add toggle button. | ||
$output .= '<button class="sub-menu-toggle" aria-expanded="false" onClick="twentytwentyoneExpandSubMenu(this)">'; | ||
$output .= '<button class="sub-menu-toggle" aria-expanded="false"' . $extra_attr . '>'; | ||
$output .= '<span class="icon-plus">' . twenty_twenty_one_get_icon_svg( 'ui', 'plus', 18 ) . '</span>'; | ||
$output .= '<span class="icon-minus">' . twenty_twenty_one_get_icon_svg( 'ui', 'minus', 18 ) . '</span>'; | ||
/* translators: Hidden accessibility text. */ | ||
|
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 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: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.
@luisherranz Follow up question: Wouldn't your suggestion call
window.localStorage.setItem
any time that any of the store'sstate
properties are updated? If not, how does it not only to look forstate.isDarkMode
changes?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.
The callback only runs when the
state.isDarkMode
value changes because it's only subscribed to thestate.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. Whenstate.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
andcontext
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luisherranz Actually, on second thought, I'm not sure that this change makes sense - from a functionality perspective:
isDarkMode
value should only be cached inlocalStorage
if it was modified by the user.isDarkMode
changes, 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.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 selectedprefers-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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, withstate.colorMode = 'system' | 'light' | 'dark'
to fully represent all the possibilities.The default value should be
system
(you can define it in PHP usingwp_interactivity_state
or in JS directly in the store definition). Then, I'd modify the watch callback to do something like this:I'm assuming you need to keep
twentytwentyoneDarkMode
for backward compatibility but if not you could usetwentytwentyoneColorMode
and storelight
/dark
.Does that make sense?
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.
@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 likesystem-light
andsystem-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 propertyisDarkModeManuallyOverwritten
(yes, very cumbersome name 😆) stores whether it's the system default or manually overwritten.See 4a35f3f
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.
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: