Interactivity API: Remove non default suffix data wp context processing.#58664
Merged
cbravobernal merged 6 commits intotrunkfrom Feb 7, 2024
Merged
Conversation
cbravobernal
added
[Type] Bug
cbravobernal
requested review from
DAreRodz,
Mamaduka,
ajitbohra,
kevin940726,
luisherranz,
nerrad and
ntwb
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 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. |
cbravobernal
changed the title
|
Size Change: -14 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
DAreRodz
reviewed
Feb 5, 2024
Contributor
DAreRodz
left a comment
DAreRodz
left a comment
There was a problem hiding this comment.
I left some suggestions, @c4rl0sbr4v0!
packages/e2e-tests/plugins/interactive-blocks/directive-context/render.php
Show resolved
Hide resolved
Comment on lines
+118
to
+121
| // If there's an error, we'll just return the inherited context. | ||
| return ( | ||
| <Provider value={ inheritedValue }>{ children }</Provider> | ||
| ); |
Contributor
There was a problem hiding this comment.
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. 🤓
Contributor
There was a problem hiding this comment.
An empty return is enough.
Comment on lines
+109
to
+116
| currentValue.current = useMemo( () => { | ||
| const newValue = deepSignal( { | ||
| [ namespace ]: value, | ||
| } ); | ||
| mergeDeepSignals( newValue, inheritedValue ); | ||
| mergeDeepSignals( currentValue.current, newValue, true ); | ||
| return currentValue.current; | ||
| }, [ inheritedValue, ...passedValues ] ); |
Contributor
There was a problem hiding this comment.
I don't expect useMemo() to throw an error. Moving this outside of the try { ... } catch would be better.
Contributor
There was a problem hiding this comment.
However, if this is intentional, then we need to include a test for that case, don't we? 🤔
Contributor
Author
There was a problem hiding this comment.
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.
cbravobernal
commented
Feb 5, 2024
Contributor
Author
Thank you very much! 🙇♂️ |
47 tasks
|
Flaky tests detected in b68c44c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7787693813
|
cbravobernal
deleted the
fix/remove-non-default-suffix-data-wp-context
branch
MaggieCabrera
added
the
[Feature] Interactivity API
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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?
Prevent from processing directives of
data-wp-contextwith other suffixes. In other directives, you may have different suffixes like:data-wp-on-document-readyordata-wp-on-document-scrollBut, in
data-wp-context, we only want developers to usedata-wp-context, notdata-wp-context--aordata-wp-context--bWhy?
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-textdirective reading that context.