Interactivity API: Allow global configs for namespaces #58749
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DAreRodz
commented
Feb 6, 2024
| @@ -26,9 +31,13 @@ store( 'core/query', { | |||
| const queryRef = ref.closest( | |||
| '.wp-block-query[data-wp-router-region]' | |||
| ); | |||
| const isDisabled = queryRef?.dataset.wpNavigationDisabled; | |||
| const { clientNavigation = true } = getConfig( 'core/router' ); | |||
There was a problem hiding this comment.
I prefer something like clientNavigationDisabled so we don't have to set a default value when the config is consumed. 😅
Also, maybe this logic should be moved to the Router's navigate?
cc: @luisherranz
There was a problem hiding this comment.
maybe this logic should be moved to the Router's navigate?
Yes, this needs to be done in the router.
There was a problem hiding this comment.
There will be some cases where the block needs to know if it's enabled or not, like in an instant search, but the rest of the time it should be automatic.
There was a problem hiding this comment.
I prefer something like
clientNavigationDisabledso we don't have to set a default value when the config is consumed.
I'm not opposed to clientNavigationDisabled, but can't this be true by default?
const { clientNavigation } = getConfig();
if (clientNavigation !== false) {
// navigate...
}There was a problem hiding this comment.
It can―it's just that, as a boolean value, it can be confusing why the following code doesn't work:
const { clientNavigation } = getConfig();
if ( clientNavigation ) {
// Oops! It doesn't navigate when undefined, but it should!
}Anyway, as long as we clarify that an undefined value should be treated as true, I'm OK with clientNavigation.
There was a problem hiding this comment.
I also prefer a shorter name, BTW. I'm only worried about potential confusion.
There was a problem hiding this comment.
Yep, let's go with clientNavigationDisabled to avoid confusion.
There was a problem hiding this comment.
Changed to clientNavigationDisabled in 7f60ebe!
|
Size Change: +4 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
47 tasks
DAreRodz
force-pushed
the
update/query-block-disable-navigation-global-config
branch
from
ee407fe
to
e0876f0
Compare
DAreRodz
added
[Type] Feature
DAreRodz
requested review from
kevin940726,
Mamaduka,
ntwb,
nerrad and
ajitbohra
as code owners
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
DAreRodz
force-pushed
the
update/query-block-disable-navigation-global-config
branch
from
61126d2
to
ca35dd7
Compare
|
Flaky tests detected in eb0ea96. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7835680217
|
DAreRodz
force-pushed
the
update/query-block-disable-navigation-global-config
branch
from
7f60ebe
to
75c3074
Compare
cbravobernal
force-pushed
the
update/query-block-disable-navigation-global-config
branch
from
75c3074
to
38b60fb
Compare
cbravobernal
reviewed
Feb 8, 2024
packages/interactivity/src/store.ts
Outdated
| return JSON.parse( storeTag.textContent ); | ||
| } catch ( e ) { | ||
| // eslint-disable-next-line no-console | ||
| console.log( e ); |
There was a problem hiding this comment.
Do we want a console.log here?
Suggested change
| console.log( e ); | |
| return {}; |
There was a problem hiding this comment.
I added a comment, as we already have a return at the end of the function. eb0ea96
DAreRodz
force-pushed
the
update/query-block-disable-navigation-global-config
branch
from
38b60fb
to
4480404
Compare
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ phpunit/blocks/render-query-test.php |
luisherranz
approved these changes
Feb 9, 2024
There was a problem hiding this comment.
Looks good to me! 🙂
I've added support for default namespaces in getConfig in 34289a9. Please, take a look before merging.
DAreRodz
deleted the
update/query-block-disable-navigation-global-config
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Epic: #56803
This PR makes the global config defined with
wp_interactivity_config()in PHP available in the client, using a new functiongetConfig( namespace )exposed from@wordpress/interactivity.Example:
Take in mind that
configdata is not reactive likestate, i.e., its changes don't trigger any element re-rendering.In this PR, we also remove the
data-wp-navigation-disabledattribute we were previously including in Query blocks to disable client-side navigations when they contain unsupported blocks. This logic is now handled with theclientNavigationproperty inside thecore/routerconfig data.Why?
Part of #56803.