-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Interactivity Router: Update for latest GB changes. #7304
Interactivity Router: Update for latest GB changes. #7304
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. |
This is building on #7405. |
1eb8e67
to
007480f
Compare
be35932
to
14cf936
Compare
14cf936
to
f2ba5de
Compare
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN:
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Update the interactivity-router package to use the @wordpress/a11y module for its aria-live regions. This was behind a Gutenberg-only flag. Since the a11y module is landing in Core in 6.7, the flag can be removed to always use the a11y module. See WordPress/wordpress-develop#7304.
Update the interactivity-router package to use the @wordpress/a11y module for its aria-live regions. This was behind a Gutenberg-only flag. Since the a11y module is landing in Core in 6.7, the flag can be removed to always use the a11y module. See WordPress/wordpress-develop#7304.
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.
Looks good to me @sirreal ! 👍
I've tested it following the instructions in the PR.
@@ -990,12 +1011,6 @@ class="wp-interactivity-router-loading-bar" | |||
data-wp-class--start-animation="state.navigation.hasStarted" | |||
data-wp-class--finish-animation="state.navigation.hasFinished" | |||
></div> | |||
<div |
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.
Sorry to bring this up after having already approved!
I've just noticed that we should rename the function to print_router_loading_markup()
, update its docstring and update the reference where we pass this function to the filter over here.
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 is a public method, extenders could be calling it or modifying its behavior in filters. Renaming the method could be disruptive. It's true the method name isn't a perfect description now. What do you think of adding a note in the method documentation instead of changing the 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.
Maybe we could do this:
- Create a new
print_router_markup
function - Deprecate the “old” method, and have it log a deprecation.
- The “old” method calls the new
print_router_markup
function.
I think that might be the way to go.
I’m looking at some existing functions/methods that have the @deprecated docstring. For example this function is deprecated, calls _deprecated_function()
and then calls the updated version:
wordpress-develop/src/wp-includes/class-wp-scripts.php
Lines 479 to 505 in 9e29083
/** | |
* Prints inline scripts registered for a specific handle. | |
* | |
* @since 4.5.0 | |
* @deprecated 6.3.0 Use methods get_inline_script_tag() or get_inline_script_data() instead. | |
* | |
* @param string $handle Name of the script to print inline scripts for. | |
* Must be lowercase. | |
* @param string $position Optional. Whether to add the inline script | |
* before the handle or after. Default 'after'. | |
* @param bool $display Optional. Whether to print the script tag | |
* instead of just returning the script data. Default true. | |
* @return string|false Script data on success, false otherwise. | |
*/ | |
public function print_inline_script( $handle, $position = 'after', $display = true ) { | |
_deprecated_function( __METHOD__, '6.3.0', 'WP_Scripts::get_inline_script_data() or WP_Scripts::get_inline_script_tag()' ); | |
$output = $this->get_inline_script_data( $handle, $position ); | |
if ( empty( $output ) ) { | |
return false; | |
} | |
if ( $display ) { | |
echo $this->get_inline_script_tag( $handle, $position ); | |
} | |
return $output; | |
} |
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.
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.
All right!
The print_router_loading_and_screen_reader_markup method name was very specific about its behavior which has changed. Rename the method to a more descriptive name and update its documentation.
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.
Looks good to me! 👍
Committed with https://core.trac.wordpress.org/changeset/59097 |
This was reverted to avoid removing the public method. New PR with these changes: #7447 |
Update the interactivity-router package to use the @wordpress/a11y module for its aria-live regions. This was behind a Gutenberg-only flag. Since the a11y module is landing in Core in 6.7, the flag can be removed to always use the a11y module. See WordPress/wordpress-develop#7304. Source: WordPress/gutenberg@7d883a4
Update the interactivity-router package to use the @wordpress/a11y module for its aria-live regions. This was behind a Gutenberg-only flag. Since the a11y module is landing in Core in 6.7, the flag can be removed to always use the a11y module. See WordPress/wordpress-develop#7304. Source: WordPress/gutenberg@20ae57d
Accounts for changes to the
@wordpress/interactivity-router
module in:Static localized strings are passed via script module data passing instead of in Interactivity API store state.
Redundant HTML used for
aria-live
regions is removed. This functionality is not handled by the@wordpress/a11y
script module added in #7405.Trac ticket: https://core.trac.wordpress.org/ticket/60647
Testing
Testing this requires package updates from WordPress/gutenberg#65539. It anticipates the package being released and updated in Core.
(Copied from WordPress/gutenberg#65539)
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.