Skip to content
This repository has been archived by the owner on Jan 15, 2019. It is now read-only.

Corrected column math in galleries - issue #91 #130

Merged
merged 6 commits into from
Oct 19, 2018

Conversation

briannaorg
Copy link
Contributor

@briannaorg briannaorg commented Oct 17, 2018

contributor: technosiren

  1. If columns are being displayed inside .entry-content, as in the gallery use case, then vw is not the correct unit of measure for the column map because the entire viewport isn't available. I switched the columns map to use percentages instead, so it's sized to its container.

  2. As pretty as the column map is in _columns.scss, the math doesn't actually work.

A single-column gallery is not equivalent to a single column width - it's equal to 12 column widths. I adjusted the math in the loop inside _galleries.scss to accommodate this, and update the map key in _columns.scss to accommodate decimal values for the actual widths required.

While testing, I discovered that the values returned for the 7 and 9 column galleries weren't mappable due to decimal places. So I ended up eliminating the columns map entirely from _galleries.scss and left it as the original map with viewport units, not currently in use.

  1. I included the gutters and inherited text alignment from the .gallery-item container so every iteration of gallery columns 1-9 is sized appropriately for its container.

@joyously
Copy link

How do I test this with the other CSS changes?

@briannaorg
Copy link
Contributor Author

How do I test this with the other CSS changes?

Either copy over _galleries.scss into your working directory and run sass, or copy over the compiled style.css from this branch to test with?

@kjellr kjellr requested a review from allancole October 17, 2018 17:32
@joyously
Copy link

Yeah, so I'd have to do that for the other changes too, to test them together...

@briannaorg
Copy link
Contributor Author

Yeah, so I'd have to do that for the other changes too, to test them together...

Not if you first checkout my branch with the fix into your working directory - then you're testing only the code in that branch.

@joyously
Copy link

I know how to test one at a time. I'm trying to test them together.

@briannaorg
Copy link
Contributor Author

Sorry, maybe posting links to the issues you're trying to test with would help - I can either merge my fixes with those or we can ask @kjellr for testing guidance.

@kjellr
Copy link
Collaborator

kjellr commented Oct 17, 2018

Which other changes are you trying to test this with, @joyously? In general, we tend to test changes one at a time, within individual branches.

@joyously
Copy link

I think it was the similar issues: #59 and #58

@briannaorg
Copy link
Contributor Author

briannaorg commented Oct 17, 2018 via email

@kjellr
Copy link
Collaborator

kjellr commented Oct 17, 2018

I think it was the similar issues: #59 and #58

Ok, thanks. I don't see any specific code changes in those issues, so I'm not sure what CSS would've been changed on your end.

In any case, this may be a product of our missing process/documentation for working with Sass while we're sorting out whether we'll be using it for sure. @allancole and I are aiming to sort that out ASAP to prevent future confusion.

@briannaorg
Copy link
Contributor Author

@allancole @kjellr this one is finally conflict-free and ready to go.

@allancole
Copy link
Collaborator

Yup, this looks good @briannaorg. Thanks for contributing! ;-)

Copy link
Collaborator

@allancole allancole left a comment

Choose a reason for hiding this comment

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

Yup, this math is much better 👍

@allancole allancole merged commit 8c168f3 into WordPress:master Oct 19, 2018
@briannaorg briannaorg deleted the fix/91 branch October 19, 2018 19:56
allancole added a commit that referenced this pull request Oct 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants