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

Reset margin and padding values for gallery #5979

Merged
merged 1 commit into from
Apr 5, 2018

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Apr 4, 2018

This PR resets margins and paddings for the gallery so the values themes set as margin and/or padding for normal lists (ul) do not interfere with the margins set in the gallery.

Fixes: #5890

The design changes depend on the theme but they should not be easy to notice and when noticed is to solve a problem where gallery used list margin of the theme.

How Has This Been Tested?

Use a theme like Twenty Fourteen that sets margin for ul's, verify the margin set by the theme is not used and the margin set in Gutenberg is used.

@jorgefilipecosta jorgefilipecosta added [Type] Bug An existing feature does not function as intended Needs Design Feedback Needs general design feedback. labels Apr 4, 2018
@jorgefilipecosta jorgefilipecosta self-assigned this Apr 4, 2018
@jorgefilipecosta jorgefilipecosta added this to the 2.6 milestone Apr 4, 2018
@jasmussen
Copy link
Contributor

If this fixes a theme issue, then it's important that we get this in. The most important aspect also seems to still be intact — the left and right margin matches that of text:

screen shot 2018-04-05 at 09 20 15

As such, this is cool.

Probably difficult, but it would nice to see a full-wide gallery be completely edge to edge. Right now you can get this:

screen shot 2018-04-05 at 09 21 11

☝️ that's a glimpse of a fullwide gallery with 2 images, a paragraph, and a fullwide image. You can see the margins on the gallery aren't idea. But I don't think that's a regression in this branch, just something that would be worth looking at if we had all the time in the world. Which we don't :D

@jorgefilipecosta jorgefilipecosta force-pushed the fix/reset-margin-gallery branch from 9e9381c to 7f5e410 Compare April 5, 2018 08:17
@jorgefilipecosta
Copy link
Member Author

Hi @jasmussen it seems like there was a rule to create the full wide effect but I think that the rule had bug .wp-block-gallery:not( .components-placeholder ):not( .alignfull )I think the "not" should not be there, I added a correction and not we should have the fullwide effect.

@jasmussen
Copy link
Contributor

Not sure the latest pushes are working, or perhaps when I tested before I was looking at the wrong branch. But I'm seeing some regressions.

screen shot 2018-04-05 at 10 39 59

Note how the left and right margins don't match the text anymore.

Also looks like the 5px margin is overridden:

screen shot 2018-04-05 at 10 40 24

This seems to affect the placeholder also:

screen shot 2018-04-05 at 10 34 26

It's easier to see if when each of those are selected:

screen shot 2018-04-05 at 10 34 34

screen shot 2018-04-05 at 10 34 38

@jorgefilipecosta jorgefilipecosta force-pushed the fix/reset-margin-gallery branch from 7f5e410 to 4325428 Compare April 5, 2018 09:16
@jorgefilipecosta
Copy link
Member Author

Hi @jasmussen, thank you for the catches, this has lots of cases. I followed your tip of using image placeholder as a reference and now the gallery looks aligned with it in all the cases.

@jasmussen
Copy link
Contributor

Wow, yep, everything seems right now. Impressive! 👍 👍

@@ -1,4 +1,4 @@
.wp-block-gallery:not( .components-placeholder ):not( .alignfull ) {
.wp-block-gallery.wp-block-gallery:not( .components-placeholder ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems the class is duplicated here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @youknowriad, the class was duplicate to increase the specify against the styles set for the front end. I added a comment to make clear that this was intentional.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other blocks we use .gutenberg .wp-block-myblock, Can we adopt the same pattern? No strong opinion about which pattern to use but just trying to be consistent.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/reset-margin-gallery branch from 4325428 to e4ae056 Compare April 5, 2018 12:53
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment about the way we add this specificity, otherwise look good to me

#5979 (comment)

…for normal lists do not change the gallery design.
@jorgefilipecosta jorgefilipecosta force-pushed the fix/reset-margin-gallery branch from e4ae056 to 026e57b Compare April 5, 2018 13:16
@jorgefilipecosta jorgefilipecosta merged commit 10ec20e into master Apr 5, 2018
@jorgefilipecosta jorgefilipecosta deleted the fix/reset-margin-gallery branch April 5, 2018 14:29
@jorgefilipecosta
Copy link
Member Author

Thank you for the tip @youknowriad, I was not aware of the pattern being used an I updated the code to follow the used pattern in other places.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Feedback Needs general design feedback. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants