Navigation Block: Apply text-decoration support as CSS class #36104
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
aaronrobertshaw
added
[Feature] Block API
|
Size Change: +309 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
|
Even if not ready, thanks for working on this 👌 |
No worries 👍 I've only had the time so far to give it a very basic test but it did work for me. So if you're feeling brave you're welcome to take it for a spin. Given the complexity of the Navigation block and all its inner workings, I wanted to be able to spend some time tomorrow to give it a more thorough test. Make sure I haven't introduced regressions to the typography support at large and that all the nested nav items etc are getting styled correctly. If that all works it will still come down to whether the approach I've taken is acceptable. To increase our chances of landing this for 5.9, I leant more towards only switching the text-decoration support to a CSS class for the Nav block only. |
I can't speak to the inner workings, I'll take your word for it. I do suspect this feature is a bit like justifications, in that simply applying the inline |
aaronrobertshaw
force-pushed
the
update/navigation-block-text-decoration
branch
from
9f38023
to
ee0f7d6
Compare
aaronrobertshaw
changed the title
aaronrobertshaw
requested review from
ellatrix,
spacedmonkey,
tellthemachines and
TimothyBJacobs
as code owners
aaronrobertshaw
commented
Nov 2, 2021
| * | ||
| * @return array Typography CSS classes and inline styles. | ||
| */ | ||
| function gutenberg_typography_generate_css_classes_and_styles( $typography_supports, $block_attributes ) { |
There was a problem hiding this comment.
I'm not sure if moving the css class and style generation to a function in this scope and using that is a good idea. I didn't see a lot of options otherwise in terms of not duplicating that logic in any dynamic block that skips serialization of block support styles but only really wishes to do so for a single feature, i.e. text decoration in this case.
@oandregal Do you have any suggestions or thoughts on this?
There was a problem hiding this comment.
If I read this correctly: we want to do this to have a helper function and avoid having to recreate all this code in navigation. If we were able to skip serialization for a single property it'd be simpler because the other properties would still be rendered by the block support function.
I think we should do this if it's helpful.
The only bit of feedback I have is about this function's signature. Once we have a function in WordPress core we can't remove it. So, at first sight, I'd make this the same as gutenberg_apply_typography_support: pass $block_type and $block_attlributes. The rationale would be that's more future proof: we don't know what we want to do in the future with this utility and perhaps we want to access the block name, the block style variations, etc. It makes it slightly more involved for your use case than passing a tweaked typography supports, but I hope that's not too much. It's in line with what we do in the client as well. What do you think of this?
There was a problem hiding this comment.
If we were able to skip serialization for a single property it'd be simpler because the other properties would still be rendered by the block support function.
I think we should do this if it's helpful.
Agreed. As we attempt to adopt block supports for more complicated blocks I can see us running into this more often.
I'll try my hand at putting together a PR to allow for this.
The only bit of feedback I have is about this function's signature. Once we have a function in WordPress core we can't remove it. So, at first sight, I'd make this the same as gutenberg_apply_typography_support: pass $block_type and $block_attlributes.
Good point. Thanks for the warning. 👍
It makes it slightly more involved for your use case than passing a tweaked typography supports, but I hope that's not too much. It's in line with what we do in the client as well. What do you think of this?
I don't think it's too much at all, better than restricting our ability to handle changes in future. I'll see how quickly the PR to allow skipping serialization of individual features comes along, then revisit this as required.
This was referenced Nov 8, 2021
7 tasks
|
Closing this in favour of skipping serialization of only the text-decoration support once #36293 becomes available. |
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.
Fixes: #34275
Description
This PR switches the application of the text-decoration block support from an inline style to a CSS class for the Navigation block. It attempts to do this with a minimum of duplication of the logic to generate the non-text-decoration typography styles.
text-decoration.getTypographyClassesAndStylesutil function to collect classes and styles in the editor as per spacing supporthas-text-decoration-underlineorhas-text-decoration-line-throughclasses..wp-block-navigation-itemelementsHow has this been tested?
Checked that typography support continued to function for:
Tested the navigation block correctly applies typography styling:
Testing Instructions
underlineorline-throughoptions and confirm only the navigation item links display that decoration visually. i.e. the placeholder text in the search block should not reflect the selected text decorationScreenshots
Types of changes
Bug fix.
Checklist:
*.native.jsfiles for terms that need renaming or removal).