Add [data-block] to appender. #35356
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
jasmussen
added
the
[Feature] Themes
|
Size Change: -30 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
I think some features use the |
|
Thanks, the test failures do appear to repeat. Before diving in to fix them, I'd love some input from the folks pinged to see if/how to proceed with the PR. |
|
This seems like a good idea. I'm not sure of the impact on older themes... |
|
It is primarily built towards older themes that may have targetted the [data-block] attribute in editor styles. |
|
Many old themes shipped different CSS to the editor and the frontend, so my concern is that the editor styles would break. That doesn't mean we shouldn't do it, just that we might need to update a lot of themes, or be ok with breaking things... |
Exactly, for example TT1 shipped this: The margin/spacing situation in the editor has been bad for a long time, prompting the need for the above in the first place. But as of Gutenberg 11.3, and for every block theme, it is no longer necessary to reset the base margin styles. So any newly developed theme should not need to include styles like the above. However for every classic theme that includes margin rules like the above, it means there will be a jump when first clicking the appender, unless we add that property. So in fact the fix here is to make those old editor styles work as they probably meant to in the first place. In that vein, this PR is more likely to fix things, than break them. The primary question is whether it's worth it, since targetting [data-block] (as opposed to |
jasmussen
force-pushed
the
try/fix-appender-shift-classic
branch
from
010d40e to
05cd10f
Compare
|
I pushed 9daedf8 in hopes that it would fix the data-block e2e test, allowing it to also count the appender as a block, but it did not seem to work. @kevin940726, I think you worked on the original test in #28160. If you have time to look, any input on how to best handle this? |
ellatrix
reviewed
Oct 13, 2021
| @@ -87,6 +87,8 @@ function BlockListAppender( { | |||
| 'block-list-appender wp-block', | |||
| className | |||
| ) } | |||
| // Many editor styles incorrectly target [data-block] instead of .wp-block. | |||
| data-block | |||
There was a problem hiding this comment.
Can we not keep the wp-block class as well?
There was a problem hiding this comment.
I remember these classes switching around many times, so it would be good to comment in depth on why which classes and attributes should be there and why some don't. :)
There was a problem hiding this comment.
I did try to remove the wp-block class in the past, but it was added back due to causing issues. There's a bit more context here: #33895 (comment)
To be honest, I am myself a little confused about the structure and the classnames, but I do understand that the empty paragraph appender, in order to emulate an actual empty paragraph, needs to be surfaced as a block. The added challenge here is how especially classic themes have come to target [data-block] as a blanket "this is a block" attribute and usually attached spacing to it.
To an extent, I wish I could fix every theme to not do that, but this isn't feasible. And avoiding the initial appender jump feels important enough to result in this PR in the first place.
What would a good comment look like? Here's a work in progress:
// The appender exists to let you add the first Paragraph before any is inserted.
// To that end, this appender should visually be presented as a block. That means
// theme CSS should style it as if it were an empty paragraph block.
// That means a `wp-block` class to ensure the width is correct, and a [data-block]
// attribute to ensure the correct margin is applied, especially for classic themes
// which have commonly targetted that attribute for margins.
What do you think?
kevin940726
reviewed
Oct 13, 2021
|
Tests are green. I'd appreciate a quick test and maybe an approval if anyone has time! |
kevin940726
approved these changes
Oct 15, 2021
|
Thanks so much. I've added the above comments for now (with better linebreaks and a typo fix), and will land this one when the checks pass. It's a very very tiny PR that very much elevates the experience for classic themes, and because it's such a small change, it's easy to revisit or revert if it turns out to cause troubles, for which I'll be here for followups. |
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.
Description
Followup to #33895 (comment). This removes a
wp-blockclass, but also adds adata-blockproperty to the appender.Due to limitations that have existed in the editor for some time, many themes have come to target [data-block] for attaching styles to "blocks", notably because the
wp-blockCSS class is not present on blocks like the Paragraph and others.The need to target [data-block] has not been there for some time, since the classic styles and resets have lowered specificity to the point that you could, and should, target elements like
pandh2etc, just like you would on the frontend. Indeed that is a goal, to not need to write editor-specific styles at all.One way in which this targetting of [data-block] manifests itself in, is in a jump when you click the first default block appender, like so:
By virtue of removing one class (so there aren't double margins competing), and indeed adding the the data-block property to the other, the shift is fixed:
I have not observed any negative side effects to this, but it fixes a very annoying problem with all the existing themes that have targetted data-block. I would love to see it land.
How has this been tested?
Please test in a classic theme and observe that when clicking the first paragraph, there's no jump or shift.
Checklist:
*.native.jsfiles for terms that need renaming or removal).