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: Fix column widths in gallery block. #2629

Merged
merged 1 commit into from Sep 4, 2017

Conversation

Projects
None yet
4 participants
@gonzomir
Contributor

gonzomir commented Aug 31, 2017

The widths of columns have wrong calc() expressions, which causes more images to fit in a row than set in the "Columns" setting of the block.

This should fix #2465

@karmatosed

This comment has been minimized.

Show comment
Hide comment
@karmatosed

karmatosed Aug 31, 2017

Member

First, with this patch I don't see any difference when looking with it applied or not, for example on a gallery of 7 items:

2017-08-31 at 15 38

Is there some step I can trace to see this? Does it rely on a number or rows? I'd love to see the impact and recreate between the two. That all said, the PR doesn't break anything.

Member

karmatosed commented Aug 31, 2017

First, with this patch I don't see any difference when looking with it applied or not, for example on a gallery of 7 items:

2017-08-31 at 15 38

Is there some step I can trace to see this? Does it rely on a number or rows? I'd love to see the impact and recreate between the two. That all said, the PR doesn't break anything.

@gonzomir

This comment has been minimized.

Show comment
Hide comment
@gonzomir

gonzomir Aug 31, 2017

Contributor

Sorry for not providing more details, I thought the issue will give enough information. Anyway, here's a gallery with 20 pictures, 5 in a row (so there must be 4 rows, all images the same size):

Before:
gutenberg-gallery-before

And after:
gutenberg-gallery-after

The problem becomes more prominent with a greater number of columns, because the calc() expressions subtract greater amounts from the width of the items, while it should always be the sum of the horizontal margins (16px in this case).

Contributor

gonzomir commented Aug 31, 2017

Sorry for not providing more details, I thought the issue will give enough information. Anyway, here's a gallery with 20 pictures, 5 in a row (so there must be 4 rows, all images the same size):

Before:
gutenberg-gallery-before

And after:
gutenberg-gallery-after

The problem becomes more prominent with a greater number of columns, because the calc() expressions subtract greater amounts from the width of the items, while it should always be the sum of the horizontal margins (16px in this case).

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Sep 4, 2017

Contributor

This looks good, thanks!

Contributor

mtias commented Sep 4, 2017

This looks good, thanks!

@mtias mtias merged commit b0df41b into WordPress:master Sep 4, 2017

2 checks passed

codecov/project 31.42% remains the same compared to 5885233
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nb

This comment has been minimized.

Show comment
Hide comment
@nb

nb Sep 5, 2017

Member

Good fix, @gonzomir – I have no idea how that mess got in :) One possible improvement would it be to extract the margins in a variable – it would make it a bit easier to follow.

Member

nb commented Sep 5, 2017

Good fix, @gonzomir – I have no idea how that mess got in :) One possible improvement would it be to extract the margins in a variable – it would make it a bit easier to follow.

@gonzomir

This comment has been minimized.

Show comment
Hide comment
@gonzomir

gonzomir Sep 8, 2017

Contributor

If the variable can be shared between components, It'll probably allow for more consistent styles. And also the calc() expressions can be joined in a mixin that generates all the columns CSS - like "give me maximum 8 columns for this base class". I'm not sure however how such approach fits in a independent components philosophy.

Contributor

gonzomir commented Sep 8, 2017

If the variable can be shared between components, It'll probably allow for more consistent styles. And also the calc() expressions can be joined in a mixin that generates all the columns CSS - like "give me maximum 8 columns for this base class". I'm not sure however how such approach fits in a independent components philosophy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment