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

Try a simpler spacing setup for gallery items #10027

Merged
merged 8 commits into from Sep 25, 2018

Conversation

Projects
None yet
6 participants
@jasmussen
Contributor

jasmussen commented 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. The columns markup itself is a little more complex, but the gallery itself is easier to style for themes, as it no longer uses negative margins.

alternate gallery

If we like this, we'll need to test for IE as well.

By the way I noticed this:

screen shot 2018-09-19 at 10 25 18

It looks like when the gallery block has no alignment, an "alignundefined" is applied.

CC: @SofiaSousa

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.

@jasmussen jasmussen self-assigned this Sep 19, 2018

@jasmussen jasmussen requested a review from WordPress/gutenberg-core Sep 19, 2018

@SofiaSousa

This comment has been minimized.

Show comment
Hide comment
@SofiaSousa

SofiaSousa Sep 19, 2018

Contributor

It solves my problem in the preview mode, no horizontal scroll anymore 👍

Contributor

SofiaSousa commented Sep 19, 2018

It solves my problem in the preview mode, no horizontal scroll anymore 👍

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 19, 2018

Contributor

@kjellr I could use your CSS sanity check on this one. Thanks.

Contributor

jasmussen commented Sep 19, 2018

@kjellr I could use your CSS sanity check on this one. Thanks.

@tofumatt

It looks like a lot of the CSS here needs to be removed rather than commented, but I tested things in IE 11 and they seemed okay.

screenshot 2018-09-19 13 01 32

I couldn't reproduce the alignundefined class, but that seems a separate issue anyway… if you can reproduce it consistently please do file a bug and I'll take a look, it'd think the fix would be straightforward.

Show outdated Hide outdated packages/block-library/src/gallery/editor.scss Outdated

@tofumatt tofumatt requested a review from WordPress/gutenberg-core Sep 19, 2018

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 19, 2018

Contributor

Thanks for the IE sanity check 💕

Contributor

jasmussen commented Sep 19, 2018

Thanks for the IE sanity check 💕

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 19, 2018

Contributor

I couldn't reproduce the alignundefined class, but that seems a separate issue anyway… if you can reproduce it consistently please do file a bug and I'll take a look, it'd think the fix would be straightforward.

Completely agree that's separate. I can reproduce it consistently though, it's only in the editor, and it appears to be benign, so it's not saved for the post itself:

undefined

Contributor

jasmussen commented Sep 19, 2018

I couldn't reproduce the alignundefined class, but that seems a separate issue anyway… if you can reproduce it consistently please do file a bug and I'll take a look, it'd think the fix would be straightforward.

Completely agree that's separate. I can reproduce it consistently though, it's only in the editor, and it appears to be benign, so it's not saved for the post itself:

undefined

Clean up the mess.
Remove debug comments. Sorry, long day.

Clarify comments.
.blocks-gallery-image,
.blocks-gallery-item {
margin: 8px;
// Add space between thumbnails, and unset right most thumbnails later.
margin: 0 $grid-size-large $grid-size-large 0;

This comment has been minimized.

@talldan

talldan Sep 20, 2018

Contributor

I think we could take advantage of aliasing $grid-size-large to make things more readable. Maybe something like $item-margin.

@talldan

talldan Sep 20, 2018

Contributor

I think we could take advantage of aliasing $grid-size-large to make things more readable. Maybe something like $item-margin.

This comment has been minimized.

@jasmussen

jasmussen Sep 21, 2018

Contributor

I would like to address/discuss this separately. We already have an $item-spacing variable of 10px that I'd like to retire in favor of a grid unit. But the overall idea is to introduce a visual grid to all of Gutenberg that should make things align better — so I would prefer we can keep the term "grid" as part of it.

@jasmussen

jasmussen Sep 21, 2018

Contributor

I would like to address/discuss this separately. We already have an $item-spacing variable of 10px that I'd like to retire in favor of a grid unit. But the overall idea is to introduce a visual grid to all of Gutenberg that should make things align better — so I would prefer we can keep the term "grid" as part of it.

@talldan

This comment has been minimized.

Show comment
Hide comment
@talldan

talldan Sep 20, 2018

Contributor

I'm definitely in favour of this approach, I think trying to only apply margin to the bottom and/or right is a constraint that results in cleaner css. 👍

Contributor

talldan commented Sep 20, 2018

I'm definitely in favour of this approach, I think trying to only apply margin to the bottom and/or right is a constraint that results in cleaner css. 👍

@kjellr

This comment has been minimized.

Show comment
Hide comment
@kjellr

kjellr Sep 20, 2018

Contributor

I like this approach too. Just a couple issues I've noticed during testing:

I'm seeing spacing issues with galleries that have 5 or more items in 4 or more rows (These persist on the front end too, and sound like they're related to @talldan's comment above):

5-items

6-items

8-items

When things drop to two columns on mobile, it looks like every second row's right margin is missing:

screen shot 2018-09-20 at 11 20 22 am

screen shot 2018-09-20 at 11 23 48 am

Contributor

kjellr commented Sep 20, 2018

I like this approach too. Just a couple issues I've noticed during testing:

I'm seeing spacing issues with galleries that have 5 or more items in 4 or more rows (These persist on the front end too, and sound like they're related to @talldan's comment above):

5-items

6-items

8-items

When things drop to two columns on mobile, it looks like every second row's right margin is missing:

screen shot 2018-09-20 at 11 20 22 am

screen shot 2018-09-20 at 11 23 48 am

@SofiaSousa

This comment has been minimized.

Show comment
Hide comment
@SofiaSousa

SofiaSousa Sep 20, 2018

Contributor

Guys, it was just a mathematical question. First subtract the margins total, then divide by the number of columns.

& .blocks-gallery-image,
& .blocks-gallery-item {
    width: calc((100% - #{ $grid-size-large }) / 2);
}

@for $i from 3 through 8 {
    &.columns-#{ $i } .blocks-gallery-image,
    &.columns-#{ $i } .blocks-gallery-item {
        width: calc((100% - #{ $grid-size-large } * #{ $i - 1 }) / #{ $i });
    }
}

2018-09-20 18 29 34
2018-09-20 18 54 40

Contributor

SofiaSousa commented Sep 20, 2018

Guys, it was just a mathematical question. First subtract the margins total, then divide by the number of columns.

& .blocks-gallery-image,
& .blocks-gallery-item {
    width: calc((100% - #{ $grid-size-large }) / 2);
}

@for $i from 3 through 8 {
    &.columns-#{ $i } .blocks-gallery-image,
    &.columns-#{ $i } .blocks-gallery-item {
        width: calc((100% - #{ $grid-size-large } * #{ $i - 1 }) / #{ $i });
    }
}

2018-09-20 18 29 34
2018-09-20 18 54 40

jasmussen added some commits Sep 21, 2018

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 21, 2018

Contributor

Thank you @SofiaSousa you were right there was something off with the math. But I believe I've fixed it now!

bugfix

Contributor

jasmussen commented Sep 21, 2018

Thank you @SofiaSousa you were right there was something off with the math. But I believe I've fixed it now!

bugfix

@jasmussen jasmussen added this to the 4.0 milestone Sep 21, 2018

@SofiaSousa

This comment has been minimized.

Show comment
Hide comment
@SofiaSousa

SofiaSousa Sep 21, 2018

Contributor

Yep, I've tried with chrome, safari, and firefox and it looks correct, thanks 👍

Contributor

SofiaSousa commented Sep 21, 2018

Yep, I've tried with chrome, safari, and firefox and it looks correct, thanks 👍

@kjellr

This comment has been minimized.

Show comment
Hide comment
@kjellr

kjellr Sep 21, 2018

Contributor

This is working great on large screens! 💯

(Please ignore this if you're already aware, but) I'm still seeing issues at mobile breakpoints. For example, this is a 3-col gallery with 4 items, as seen on a small screen:

screen shot 2018-09-21 at 8 27 38 am

Contributor

kjellr commented Sep 21, 2018

This is working great on large screens! 💯

(Please ignore this if you're already aware, but) I'm still seeing issues at mobile breakpoints. For example, this is a 3-col gallery with 4 items, as seen on a small screen:

screen shot 2018-09-21 at 8 27 38 am

@talldan

This comment has been minimized.

Show comment
Hide comment
@talldan

talldan Sep 21, 2018

Contributor

It looks like the margin reset is still being applied to every nth item for the mobile fallback. That's a bit of a tricky one to solve.

Contributor

talldan commented Sep 21, 2018

It looks like the margin reset is still being applied to every nth item for the mobile fallback. That's a bit of a tricky one to solve.

@SofiaSousa

This comment has been minimized.

Show comment
Hide comment
@SofiaSousa

SofiaSousa Sep 21, 2018

Contributor

Still think math is not totally correct:

calc((100% / 2) - #{ $grid-size-large })

is not the same of

calc((100% - #{ $grid-size-large }) / 2)

With a practical example, assuming 100% is 580px:

580/2-16 = 274
(580-16)/2 = 282

Now, the columns width only looks right because of the flex-wrap: 1.


The width of a column is the total width less the total of margin between columns divide by the number of columns.

the total width = 100%
the total of margin = margin size * number of spaces
number of spaces = number of columns - 1

column width = (100% - (margin size * number of columns - 1)) / number of columns

When colunms = 2

width = (100% - 16px) / 2  

When colunms = 3

width = (100% - 32px) / 3 

So my solution would be something like this:

  • Removing right margin of the last column (for 1 or 2 columns)
// On mobile and responsive viewports, we allow only 1 or 2 columns at the most.
& .blocks-gallery-image,
& .blocks-gallery-item {
    width: calc((100% - #{ $grid-size-large }) / 2);

    &:nth-child(odd){            
         margin-right: 0;
    }
}

&.columns-1 .blocks-gallery-image,
&.columns-1 .blocks-gallery-item {
    width: 100%;
    margin-right: 0;
}
  • Setting right margin for all items
  • Finally, only apply n columns margin rules beyond mobile viewports
// Beyond mobile viewports, we allow up to 8 columns.
@include break-small {
    @for $i from 3 through 8 {
        &.columns-#{ $i } .blocks-gallery-image,
        &.columns-#{ $i } .blocks-gallery-item {
            width: calc((100% - #{ $grid-size-large } * #{ $i - 1 }) / #{ $i });
            margin-right: 16px;
        }
    }

    // Unset the right margin on every rightmost gallery item to ensure center balance.
    @for $column-count from 1 through 8 {
        &.columns-#{ $column-count } .blocks-gallery-image:nth-of-type(#{ $column-count }n),
        &.columns-#{ $column-count } .blocks-gallery-item:nth-of-type(#{ $column-count }n) {
            margin-right: 0;
        }
    }
}

2018-09-21 15 53 37

2018-09-21 15 56 14

2018-09-21 15 56 39

Contributor

SofiaSousa commented Sep 21, 2018

Still think math is not totally correct:

calc((100% / 2) - #{ $grid-size-large })

is not the same of

calc((100% - #{ $grid-size-large }) / 2)

With a practical example, assuming 100% is 580px:

580/2-16 = 274
(580-16)/2 = 282

Now, the columns width only looks right because of the flex-wrap: 1.


The width of a column is the total width less the total of margin between columns divide by the number of columns.

the total width = 100%
the total of margin = margin size * number of spaces
number of spaces = number of columns - 1

column width = (100% - (margin size * number of columns - 1)) / number of columns

When colunms = 2

width = (100% - 16px) / 2  

When colunms = 3

width = (100% - 32px) / 3 

So my solution would be something like this:

  • Removing right margin of the last column (for 1 or 2 columns)
// On mobile and responsive viewports, we allow only 1 or 2 columns at the most.
& .blocks-gallery-image,
& .blocks-gallery-item {
    width: calc((100% - #{ $grid-size-large }) / 2);

    &:nth-child(odd){            
         margin-right: 0;
    }
}

&.columns-1 .blocks-gallery-image,
&.columns-1 .blocks-gallery-item {
    width: 100%;
    margin-right: 0;
}
  • Setting right margin for all items
  • Finally, only apply n columns margin rules beyond mobile viewports
// Beyond mobile viewports, we allow up to 8 columns.
@include break-small {
    @for $i from 3 through 8 {
        &.columns-#{ $i } .blocks-gallery-image,
        &.columns-#{ $i } .blocks-gallery-item {
            width: calc((100% - #{ $grid-size-large } * #{ $i - 1 }) / #{ $i });
            margin-right: 16px;
        }
    }

    // Unset the right margin on every rightmost gallery item to ensure center balance.
    @for $column-count from 1 through 8 {
        &.columns-#{ $column-count } .blocks-gallery-image:nth-of-type(#{ $column-count }n),
        &.columns-#{ $column-count } .blocks-gallery-item:nth-of-type(#{ $column-count }n) {
            margin-right: 0;
        }
    }
}

2018-09-21 15 53 37

2018-09-21 15 56 14

2018-09-21 15 56 39

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

Co-authored-by: Sofia Sousa <csofiamsousa@gmail.com>
@talldan

This comment has been minimized.

Show comment
Hide comment
@talldan

talldan Sep 21, 2018

Contributor

@SofiaSousa Thanks! that works well for me. I've pushed the CSS from your comment in a commit. @kjellr - are you able to retest? I had a quick test and couldn't spot any issues.

Contributor

talldan commented Sep 21, 2018

@SofiaSousa Thanks! that works well for me. I've pushed the CSS from your comment in a commit. @kjellr - are you able to retest? I had a quick test and couldn't spot any issues.

@kjellr

This comment has been minimized.

Show comment
Hide comment
@kjellr

kjellr Sep 21, 2018

Contributor

@kjellr - are you able to retest? I had a quick test and couldn't spot any issues.

Looks good in Gutenberg!

screen shot 2018-09-21 at 1 13 42 pm

I think that latest update needs to get pushed to the font-end styles too though:

screen shot 2018-09-21 at 1 15 05 pm

Contributor

kjellr commented Sep 21, 2018

@kjellr - are you able to retest? I had a quick test and couldn't spot any issues.

Looks good in Gutenberg!

screen shot 2018-09-21 at 1 13 42 pm

I think that latest update needs to get pushed to the font-end styles too though:

screen shot 2018-09-21 at 1 15 05 pm

@gziolo gziolo referenced this pull request Sep 24, 2018

Closed

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

4 of 4 tasks complete
@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 24, 2018

Contributor

Hmm @kjellr I'm not seeing that issue on the front-end. Which theme is this? Also the rules should be output on the frontend since they're in style.scss. Cache issue? Otherwise let me know how to reproduce. Thanks for testing.

Contributor

jasmussen commented Sep 24, 2018

Hmm @kjellr I'm not seeing that issue on the front-end. Which theme is this? Also the rules should be output on the frontend since they're in style.scss. Cache issue? Otherwise let me know how to reproduce. Thanks for testing.

@talldan

This comment has been minimized.

Show comment
Hide comment
@talldan

talldan Sep 24, 2018

Contributor

Also looks good to me in the front-end 👍

Contributor

talldan commented Sep 24, 2018

Also looks good to me in the front-end 👍

@kjellr

This comment has been minimized.

Show comment
Hide comment
@kjellr

kjellr Sep 24, 2018

Contributor

Very odd! I'm still seeing the error. It doesn't seem like a caching bug, since it's happening in every browser I try — even after changing themes and creating brand new galleries. Here's what I'm doing:

  1. Open a new post
  2. Add a gallery with two items (screenshot)
  3. Publish, and view on the front end (screenshot)

Those screenshots are in Safari 12.0 (using the Twenty Seventeen theme), but I'm seeing the same in Chrome + FF, too. 🤔

Contributor

kjellr commented Sep 24, 2018

Very odd! I'm still seeing the error. It doesn't seem like a caching bug, since it's happening in every browser I try — even after changing themes and creating brand new galleries. Here's what I'm doing:

  1. Open a new post
  2. Add a gallery with two items (screenshot)
  3. Publish, and view on the front end (screenshot)

Those screenshots are in Safari 12.0 (using the Twenty Seventeen theme), but I'm seeing the same in Chrome + FF, too. 🤔

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 25, 2018

Contributor

I still can't reproduce. Tried Safari, tried Twenty Seventeen. This is so weird.

Try this:

git checkout master
git pull
git branch -D try/gallery-margins-refactor
git checkout try/gallery-margins-refactor
npm run dev

It's sort of a nuclear way to delete the branch, pull it fresh and try it again. My only thought is you have some cache ghost or an old version of the branch.

Contributor

jasmussen commented Sep 25, 2018

I still can't reproduce. Tried Safari, tried Twenty Seventeen. This is so weird.

Try this:

git checkout master
git pull
git branch -D try/gallery-margins-refactor
git checkout try/gallery-margins-refactor
npm run dev

It's sort of a nuclear way to delete the branch, pull it fresh and try it again. My only thought is you have some cache ghost or an old version of the branch.

@SofiaSousa

This comment has been minimized.

Show comment
Hide comment
@SofiaSousa

SofiaSousa Sep 25, 2018

Contributor

I have try/gallery-margin-refactor last changes and also got this for a 2 columns gallery:

screen shot 2018-09-25 at 09 54 11

screen shot 2018-09-25 at 09 54 49

This is chrome but the same happens on safari or firefox. And it happens because of this rule:

.wp-block-gallery .blocks-gallery-image:nth-child(odd), 
.wp-block-gallery .blocks-gallery-item:nth-child(odd) {
    margin-right: 0;
}
Contributor

SofiaSousa commented Sep 25, 2018

I have try/gallery-margin-refactor last changes and also got this for a 2 columns gallery:

screen shot 2018-09-25 at 09 54 11

screen shot 2018-09-25 at 09 54 49

This is chrome but the same happens on safari or firefox. And it happens because of this rule:

.wp-block-gallery .blocks-gallery-image:nth-child(odd), 
.wp-block-gallery .blocks-gallery-item:nth-child(odd) {
    margin-right: 0;
}
@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 25, 2018

Contributor

You were right! I could reproduce it with a gallery of 2 items, and removing that rule fixed it. Thank you!

Contributor

jasmussen commented Sep 25, 2018

You were right! I could reproduce it with a gallery of 2 items, and removing that rule fixed it. Thank you!

@SofiaSousa

This comment has been minimized.

Show comment
Hide comment
@SofiaSousa

SofiaSousa Sep 25, 2018

Contributor

@jasmussen

I'm sorry guy, but you can't simply remove that rule, it's needed for mobile.

:nth-child(odd) should be replaced by &:nth-of-type(even).

Contributor

SofiaSousa commented Sep 25, 2018

@jasmussen

I'm sorry guy, but you can't simply remove that rule, it's needed for mobile.

:nth-child(odd) should be replaced by &:nth-of-type(even).

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 25, 2018

Contributor

I'm sorry guy, but you can't simply remove that rule, it's needed for mobile.

Haha, sorry — It seemed to work for me in mobile in quick testing, but I've now tested with additional configurations and breakpoints. Added your revised rule. Thank you. Good for final round of testing?

Contributor

jasmussen commented Sep 25, 2018

I'm sorry guy, but you can't simply remove that rule, it's needed for mobile.

Haha, sorry — It seemed to work for me in mobile in quick testing, but I've now tested with additional configurations and breakpoints. Added your revised rule. Thank you. Good for final round of testing?

@SofiaSousa

This comment has been minimized.

Show comment
Hide comment
@SofiaSousa

SofiaSousa Sep 25, 2018

Contributor

Well, it's looking nice in safari, firefox and chrome for me, but more people should test, just in case :)

Contributor

SofiaSousa commented Sep 25, 2018

Well, it's looking nice in safari, firefox and chrome for me, but more people should test, just in case :)

@kjellr kjellr self-requested a review Sep 25, 2018

@kjellr

kjellr approved these changes Sep 25, 2018

This is working great for me now. Thanks, everyone!

@jasmussen jasmussen merged commit 3f36aba into master Sep 25, 2018

2 checks passed

codecov/project 48.83% (+0.05%) compared to 13bee70
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasmussen jasmussen deleted the try/gallery-margins-refactor branch Sep 25, 2018

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Sep 25, 2018

Contributor

Nice work here all, it was fun reading the history :)

Contributor

mtias commented Sep 25, 2018

Nice work here all, it was fun reading the history :)

kjellr added a commit that referenced this pull request Sep 27, 2018

Fix gallery margin while typing.
#10027 introduced a small bug where the last item in a gallery jumps around when a user is in the process of adding a caption.

This is due to a `:nth-last-child(2)` style rule that is applied to a gallery when it has an `is-selected` class, but not an `is-typing` class.

This PR modifies that rule to include the `is-typing` class and remove the jump.

kjellr added a commit that referenced this pull request Sep 28, 2018

Fix gallery margin while typing. (#10221)
#10027 introduced a small bug where the last item in a gallery jumps around when a user is in the process of adding a caption.

This is due to a `:nth-last-child(2)` style rule that is applied to a gallery when it has an `is-selected` class, but not an `is-typing` class.

This PR modifies that rule to include the `is-typing` class and remove the jump.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment