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

Remove excess <p> from Tag Cloud edit function #63716

Closed
richtabor opened this issue Jul 18, 2024 · 11 comments · Fixed by #63832
Closed

Remove excess <p> from Tag Cloud edit function #63716

richtabor opened this issue Jul 18, 2024 · 11 comments · Fixed by #63832
Assignees
Labels
[Block] Tag Cloud Affects the Tag Cloud Block [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@richtabor
Copy link
Member

richtabor commented Jul 18, 2024

In the edit function of the Tag Cloud block, there is a secondary <p> within the block, around the tags. This does not occur on the front-end. The extra <p> within the block is causing additional default browser margin to be applied within the block only within the editor. The front-end looks as expected.

I propose the edit function is updated to accurately reflect the front end, which will bring these closer to 1:1.

Editor

CleanShot 2024-07-18 at 12 39 49

Frontend

CleanShot 2024-07-18 at 12 41 49

@richtabor richtabor added [Type] Bug An existing feature does not function as intended [Block] Tag Cloud Affects the Tag Cloud Block labels Jul 18, 2024
@richtabor richtabor added this to Polish Jul 18, 2024
@Mamaduka
Copy link
Member

The block uses server-side rendering; the tag cloud markup is the same on both ends as a single <p> element wrapper.

@richtabor
Copy link
Member Author

Right, but a block wrapper exists on the edit function, which moves the p inside, which causes it to get margin applied.

@akasunil
Copy link
Member

Came here to report this issue. Glad to find out it already been noticed.
This also causes problems with this global style support for this block.

image

@aaronrobertshaw aaronrobertshaw self-assigned this Jul 22, 2024
@aaronrobertshaw
Copy link
Contributor

I can take a look into this one this week but I'm not sure there is a true fix unless we rewrite the Tag Cloud edit JS component to use the REST API for everything.

A while ago, there was an effort to reduce some of the double-up of padding/margin etc when block supports were applied to server-side rendered blocks. The following PRs show a bit of that history.

The way the server-side rendering works, I don't think the wrapper can be removed.

There might be two compromise options to move forward in the short term:

  1. Remove the class name from the inner <p> tag and add some editor styles to make it as benign as possible.
  2. Add editor styling to try and counter global styles on the wrapper instead

Any thoughts?

Is there enough value in refactoring this block to use the REST API to warrant the dev time? There may need to be some updates to the REST API too if any functionality is missing.

@richtabor
Copy link
Member Author

Maybe the simplest solution is to add block css to negate the

margin within the editor.

@ramonjd
Copy link
Member

ramonjd commented Jul 23, 2024

Add editor styling to try and counter global styles on the wrapper instead

Second this!

@aaronrobertshaw
Copy link
Contributor

A tentative fix is up in #63832.

Ultimately, I chose to apply the resets to the inner element rather than the wrapper because alignment classes with default padding are only applied on the outer wrapper. The approach in #63832 minimizes the required changes.

@akasunil
Copy link
Member

@aaronrobertshaw we might have to do same for other block support attribute styles like background, colors, border etc.

@aaronrobertshaw
Copy link
Contributor

Thanks @akasunil, this issue is a little more slanted towards the spacing issues caused by default and global styles.

Block supports should already be taken care of by the skipBlockSupportAttributes on the server-side render component here. I believe global styles are ok given the class name on the inner wrapper.

If there is a problem with the skipping of block support attributes and classes that would be best as a separate issue as it would affect all blocks using server-side rendering. To be able to effectively debug further we need some steps to replicate the issue and a screenshot that shows the conflicting styles.

@akasunil
Copy link
Member

akasunil commented Jul 25, 2024

@aaronrobertshaw I used the Tag Cloud Block to test color support, and I found that block and styles were not working properly. Here is video for reference.

Blog-Home-.-Template-.-gutenberg-.-Editor-.-WordPress.8.webm

It appears that block style takes precedence over global styles applied to the block, it should be the other way around.

Block style Global style
image image

@aaronrobertshaw
Copy link
Contributor

@akasunil this discussion might be better over on your PR proposing to add color support to the Tag Cloud block.

Here is video for reference.

It appears as though this video is highlighting the spacing issue #63832 addresses in a superficial manner. I've only just merged that PR a minute ago so it should be resolved now.

It appears that block style takes precedence over global styles applied to the block, it should be the other way around.

I think this is back-to-front. Block instance styles should take precedence over global styles.

The issue is that the color block support classes and inline styles aren't being inherited by the server-side rendered block markup. The best approach there might be to follow what was done for the Latest Comments block. Rather than skip all block support attributes, the block manually filters problematic support styles (e.g. spacing) out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Tag Cloud Affects the Tag Cloud Block [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants