Add runtime support for the wp-style directive
#52645
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
Co-authored-by: Carlos Bravo <c4rl0sbr4v0@users.noreply.github.com>
luisherranz
added
[Type] Enhancement
luisherranz
commented
Jul 14, 2023
packages/e2e-tests/plugins/interactive-blocks/directive-style/block.json
Show resolved
Hide resolved
|
Size Change: +262 B (0%) Total Size: 1.43 MB
ℹ️ View Unchanged
|
luisherranz
requested review from
kevin940726,
Mamaduka,
ntwb,
nerrad and
ajitbohra
as code owners
SantosGuillamot
approved these changes
Jul 18, 2023
There was a problem hiding this comment.
I've tested it, and it seems to be working great with context, state, and selectors 🙂
I just have one question: If I have the same property (color for example) defined in inline styles and in the wp-style directive, which one should prevail?
From my tests, it seems the directive takes over the inline styles. Not saying it is wrong, not sure which is the best approach, I just would like to understand the reasoning behind the decision. Have we considered how this might impact global styles? If I am not mistaken many of them are adding inline styles. Should it be the block developer's responsibility?
On the other hand, I think we could use this directive in part of the lightbox implementation and, in that case, it could be handy that the directive prevails over the inline styles so we are able to override them once the lightbox is opened.
|
Thanks, Mario 🙂
It's not taking over the inline styles. The Or am I missing something? |
|
I agree with this sentence:
The directive should be the source of truth in this case 🤔 |
I guess it makes sense 🙂 I was asking because I didn't think too much about it, in case you had considered different scenarios. I was just thinking that, depending on the implementation, it could cause unexpected UX combined with the Site Editor styles. But I guess that's the block's responsibility. Maybe they can even offer settings to decide the behavior. What if I want to keep the inline style until an action happens (a button is clicked for example)? Is that easy to do? If I am not mistaken, if I return null the style is going to be deleted, right? With something like: selectors.color: ( { state } ) => {
return state.isOpen ? 'red' : null
} |
I guess they could get the initial inline style color using |
|
Thanks for the reviews, by the way 🙂 Merging this now 🎉 |
8 tasks
10 tasks
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?
Add runtime support for the
wp-styledirective.Co-authored-by @c4rl0sbr4v0.
Why?
Because it's one of the core directives and it was still missing.
How?
By adding it to the list of directives.
I used a magic function 🪄 from Goober (🥜) made by @cristianbote 🧙♂️ to convert a CSS string into a style object.
Testing Instructions
data-wp-styledirective to a block and reference some part of the state/context.styleattribute changed accordingly.