Skip to content
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

Add has-custom-padding class to cover block with custom padding. #30074

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

getsource
Copy link
Member

Description

Adds has-custom-padding class to the cover block when custom padding is applied and provides a deprecation for the change.

This updates the existing padding feature by adding a class so that a theme's default padding styles can be ignored when custom padding is present.

I'm not sure if it's best to do it within each block or higher up/elsewhere, so that custom blocks that support padding don't have to add the class manually. Presenting this approach for review and to continue discussion.

If this is merged, recommend considering changes for the other library blocks with padding support, which look to be group and verse.

See: #24337
Props for feedback/review from @Mamaduka.

How has this been tested?

wp-env -- by setting custom padding on a theme supporting it.

I created a cover block before applying the code change, and verified that the deprecation would handle the addition of the class properly.

I created a cover block after applying the change, and verified the class was added when custom padding was applied.

Ran npm run test locally, which passes.

Screenshots

n/a

Types of changes

Update to existing feature. Requires deprecation, due to adding a class, which is included.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@getsource getsource added [Block] Cover Affects the Cover Block - used to display content laid over a background image [Type] Enhancement A suggestion for improvement. labels Mar 22, 2021
@youknowriad
Copy link
Contributor

I'm not sure I like this change personally. Because with this logic, any custom style applied need to have a dedicated class (thinking about margins, colors, gradients, font sizes, text transform, line height...) I think it's just too much personally.

@ntsekouras
Copy link
Contributor

This updates the existing padding feature by adding a class so that a theme's default padding styles can be ignored when custom padding is present.

Isn't that the case already since the applied padding styles are inline?

@oandregal
Copy link
Member

I've looked at the upstream issue godaddy-wordpress/go#541 that prompted to create #24337 and it looks like what happened was that the user styles weren't applied. The only reasons I can think of for that to happen is that 1) it was an early bug? or 2) the theme was using !important to set the padding? I wonder if it's still an issue.

There's also the weird case in which a theme sets padding for all sub-properties (top, right, bottom, left) but the user only sets it for one. This is not an issue in my view, it may be a valid use case and the user can still set the padding for the other ones as well.

For context, I've seen that we only attach classes to "signal" the presence of styles for colors: has-text-color, has-background, and has-background-dim (cover block). I'm not 100% certain, but I vaguely remember having read somewhere that it was done to allow themes to add margin/padding under certain color combinations? The rest of the style properties don't have these classes.

@getsource
Copy link
Member Author

I'm not sure I like this change personally. Because with this logic, any custom style applied need to have a dedicated class (thinking about margins, colors, gradients, font sizes, text transform, line height...) I think it's just too much personally.

That reasoning makes sense to me!
Is there a different/better way to make sure that user choice is respected, without adding classes?

@nosolosw: Yes! It's possible that was fixed.
I think the primary concern is this one:
godaddy-wordpress/go#541 (comment)
<snip>...that way we can check for that class and contain our padding styles as the default/standard application (as it should be) and if a user applies a custom padding, it'll be 100% right.

I can do a little research on the other classes and what use cases they were added for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants