search

Automattic / jetpack

Security, performance, marketing, and design tools — Jetpack is made by WordPress experts to make WP sites safer and faster, and help you grow your traffic.
https://jetpack.com/
Other
1.58k stars 798 forks source link

Shortcodes: Remove margins #10485

Open kraftbj opened 5 years ago

kraftbj commented 5 years ago

With the legacy shortcode on Twenty Nineteen, the defined margins make things look poor.

Steps to reproduce the issue

  1. Add a recipe shortcode on Twenty Nineteen

What I expected

Something like this: 2018-10-30 at 08 57

What happened instead

2018-10-30 at 08 57

This is due to the margin set for the .jetpack-recipe class margin: 1.5em 1%. I'm not terribly sure if this is something we should report and patch in Twenty Nineteen or in Jetpack. Thoughts?

cc: @allancole

jeherve commented 5 years ago

Related: #10390

kraftbj commented 5 years ago

To add on why I'm wondering about it being a Twenty Nineteen issue -- how many other plugins, embeds, etc, will set a margin expecting the normal "content width" to be set by the theme, as opposed to here where it seems like it is up to each added element to set the overall margin compared to full-width?

crunnells commented 5 years ago

This is going to be an issue with most Gutenberg themes moving forward. The implementation Twenty Nineteen uses to support wide and full-width images is one we're using for many of our new Gutenberg themes. As long as we're aware of which shortcodes need to have their margins overridden, we can make sure they're addressed in the theme CSS.

Alternatively, we could take out the margins for some of these blocks, so that we have less work moving forward.

kraftbj commented 5 years ago

Alternatively, we could take out the margins for some of these blocks, so that we have less work moving forward.

Long-term, it doesn't make sense to ask theme developers to write in custom CSS for every random plugin's shortcode. Re-opening to remove our CSS if this is going to be generally how themes work moving forward.

allancole commented 5 years ago

@kraftbj Yeah, this is a pretty tricky issue to cover as there’s many different ways to approach it. Twenty Nineteen (and some other a8c themes) expects anything that appears within the_content() to behave like a “Block.” Block styles are applied modularly (sort of) and contained to the limits of it’s outermost wrapper. Setting margins, floats or positioning styles on that outermost wrapper, means the theme won’t be able to deal with it in a uniform way.

When the recipe shortcode is replaced by its Gutenberg Block equivalent, it’ll automatically play nice with the theme, so with that in mind, I think the shortcode should work similarly and not come with styles on it’s outermost wrapper. But I’m open to further discussion on this as I’m not sure if changing that now would negatively effect other themes.

stale[bot] commented 5 years ago

This issue has been marked as stale. This happened because:

No further action is needed. But it's worth checking if this ticket has clear reproduction steps and it is still reproducible. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.

crunnells commented 5 years ago

This is still an issue

stale[bot] commented 4 years ago

This issue has been marked as stale. This happened because:

No further action is needed. But it's worth checking if this ticket has clear reproduction steps and it is still reproducible. Feel free to close this issue if you think it's not valid anymore — if you do, please add a brief explanation.