-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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 API: Remove non default suffix data wp context processing. #58664
Interactivity API: Remove non default suffix data wp context processing. #58664
Conversation
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 SVNCore Committers: Use this line as a base for the props when committing in SVN:
GitHub Merge commitsIf 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. |
Size Change: -14 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
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 left some suggestions, @c4rl0sbr4v0!
packages/e2e-tests/plugins/interactive-blocks/directive-context/render.php
Show resolved
Hide resolved
// If there's an error, we'll just return the inherited context. | ||
return ( | ||
<Provider value={ inheritedValue }>{ children }</Provider> | ||
); |
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.
We don't need to render another Provider
as the inherited value already comes from a Provider
. Actually, we can skip this part entirely because the directives don't need to return anything. 🤓
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.
An empty return
is enough.
currentValue.current = useMemo( () => { | ||
const newValue = deepSignal( { | ||
[ namespace ]: value, | ||
} ); | ||
mergeDeepSignals( newValue, inheritedValue ); | ||
mergeDeepSignals( currentValue.current, newValue, true ); | ||
return currentValue.current; | ||
}, [ inheritedValue, ...passedValues ] ); |
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 don't expect useMemo()
to throw an error. Moving this outside of the try { ... } catch
would be better.
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.
However, if this is intentional, then we need to include a test for that case, don't we? 🤔
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 previously cause there was errors of trying to map an undefined variable.
As we are now removing:
.map( ( c ) => deepSignal( { [ c.namespace ]: c.value } ) )
.reduceRight( mergeDeepSignals );
That possible error has gone. Now it works fine with two contexts. As you mentioned to add to the test.
Thank you very much! 🙇♂️ |
Flaky tests detected in b68c44c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7787693813
|
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.
LGTM!
What?
Prevent from processing directives of
data-wp-context
with other suffixes. In other directives, you may have different suffixes like:data-wp-on-document-ready
ordata-wp-on-document-scroll
But, in
data-wp-context
, we only want developers to usedata-wp-context
, notdata-wp-context--a
ordata-wp-context--b
Why?
Because there is no need to overcomplicate things. Contexts are unique per element, and you can always add more elements to the already existing context rather than creting new ones.
Testing Instructions
data-wp-context-whatever
.wp-text
directive reading that context.