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

Gallery: Use space-between instead of negative margins #9670

Closed

Conversation

SofiaSousa
Copy link
Contributor

Description

Using justify-content: space-between in gallery block instead of negative margins. Also removed margins of gallery items.

Added missing alignment rules for galleries.

How has this been tested?

In the browser, adding 3 images to a gallery block for example and checking the width and space between images.

Types of changes

Just style.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Hi @SofiaSousa thank you for your contribution 👍
I think one of the reasons for the use of negative margins was to align the gallery with the placeholder and with the content of other blocks, e.g., the paragraph.
And with this changes galleries are a little bit out of the content area.
Before:
screen shot 2018-09-06 at 20 27 25

After:
screen shot 2018-09-06 at 20 26 49

One option we have to make the gallery look inside the content area is move the margins to the editor.scss file so they only affect the editor, and on the front end, we avoid the negative margins.

On the front end, the difference I noticed is that the space between the images got a little bit bigger.

@jasmussen, @karmatosed would it be possible to have a look at the design changes.

@jasmussen
Copy link
Contributor

Thanks so much for the PR!

From the branch name, try/avoid-gallery-x-scroll it seems like you were running into trouble with horizontal overflow is that correct? Were there scrollbars in situations there shouldn't be?

Jorge is right, the reason for the negative margins was to ensure the gallery was both spaced evenly between images, but also was only as wide as the main column of text and placeholders.

I'm not opposed to using space-between or more flexiness, but I need to understand the problem this is trying to solve first. I'm notably a little concerned about the new float rules (https://github.com/WordPress/gutenberg/pull/9670/files#diff-9ffb7855d1c617407b369408c6c01fe4R99), given we've had some challenges with floats in the first case.

@SofiaSousa
Copy link
Contributor Author

Well we are trying to use gutenberg outside of the box, I mean outside of the wordpress and wordpress core style.

The margins in blocks-gallery-item make this 'gap' at the right edge which negative margin of wp-block-gallery doesn't solve, and the result is the horizontal scroll.

image

So the solution I saw was removing wp-block-gallery and blocks-gallery-item margins and play with flexbox.

@jasmussen
Copy link
Contributor

Hmm that's a good point.

I will think about how this can be done, but a plan B could be to move those margin styles to theme.scss as "opinionated styles" that themes have to opt into.

jasmussen pushed a commit that referenced this pull request Sep 19, 2018
The stock gallery features white gutters between items. Currently in `master`, every item has an even 8px margin all around. We then compensate for this, left and right, of the gallery, to make sure items in the gallery align correctly with text width above and below the gallery.

This PR attempts a different approach. It sets only the right and bottom margin on gallery items, and then unsets the right margin on the rightmost item in each row.

This is a little more complex in the rules that are output, but it solves the issue presented here: #9670 (comment)

Let me know your thoughts.
@gziolo
Copy link
Member

gziolo commented Sep 24, 2018

@jasmussen @SofiaSousa, should we close this one in favor of #10027?

@SofiaSousa
Copy link
Contributor Author

@jasmussen yes, thanks!

@SofiaSousa SofiaSousa closed this Sep 24, 2018
@SofiaSousa SofiaSousa deleted the try/avoid-gallery-x-scroll branch September 24, 2018 08:53
jasmussen added a commit that referenced this pull request Sep 25, 2018
* Try a simpler spacing setup for gallery items

The stock gallery features white gutters between items. Currently in `master`, every item has an even 8px margin all around. We then compensate for this, left and right, of the gallery, to make sure items in the gallery align correctly with text width above and below the gallery.

This PR attempts a different approach. It sets only the right and bottom margin on gallery items, and then unsets the right margin on the rightmost item in each row.

This is a little more complex in the rules that are output, but it solves the issue presented here: #9670 (comment)

Let me know your thoughts.

* Clean up the mess.

Remove debug comments. Sorry, long day.

Clarify comments.

* Address feedback.

* Address feedback.

* Bugfix.

* Adjust gallery css layout math, and ensure margin is reset correctly at mobile breakpoints

Co-authored-by: Sofia Sousa <[email protected]>

* Fix odd rule

Props @SofiaSousa

* Add fixed rule
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants