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

fix(layout-grid): use correct values for margins vs gutters #4494

Merged
merged 3 commits into from
Sep 2, 2021

Conversation

phou87
Copy link
Contributor

@phou87 phou87 commented Sep 1, 2021

Fixes the values used for margins and gutters in layout-grid

Description

The current implementation of layout-grid incorrectly retains gutters for every cell, even the first and last one, and adjusts the margin of the grid to compensate. For many grids this works fine, but not all of them. The simplest example that exposes the flaws of this implementation is any layout-grid with gridMargins of 0 - any such grid will still have some margin due to the left gutter of the first cell and the right gutter of the last cell.

The below image illustrates the current implementation of the grid:

Screen Shot 2021-09-01 at 2 20 59 PM

The below image illustrates what the expected implementation of the grid is:

Screen Shot 2021-09-01 at 2 20 56 PM

This PR fixes this issue by correctly differentiating edge cases for the first and last cell. Unfortunately, the calculations for cell width are actually easier with the wrong implementation, since a cell's expected width is a direct ratio of the incorrect grid width, so the width calculations with the correct implementation become more complex.

For example, assume in the above illustrated grid that the grid's width is 600px, the margins are 100px each, and the gutters are 50px. That means that each cell should be 100px wide, and each cell including its gutters should be 150px wide. This width can easily be calculated using the ratio of a cell's span divided by the total number of columns multiplied by the grid width (in this case, 1/3 * 450px for the center cell, with a slight adjustment for the edge cells). In the correct implementation, the grid's width is now 400px, which is not as mathematically translatable to the desired width.

Scope

Patch: Bug Fix

@vercel
Copy link

vercel bot commented Sep 1, 2021

@phou87 is attempting to deploy a commit to the Uber UI Platform Team on Vercel.

A member of the Team first needs to authorize it.

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 1, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 9a6d03c:

Sandbox Source
Basic usage Configuration

@vercel
Copy link

vercel bot commented Sep 1, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/uber-ui-platform/baseweb/CyY9w9zR1gyYpJE2J4dgmLb9gimZ
✅ Preview: https://baseweb-git-fork-phou87-fix-layout-grid-c415b6-uber-ui-platform.vercel.app

@UberOpenSourceBot
Copy link
Contributor

Visual changes were detected on this branch. Please review the following PR containing updated snapshots: phou87#2

…gins

test(vrt): update visual snapshots for phou87:fix-layout-grid-margins [skip ci]
Copy link
Collaborator

@chasestarr chasestarr left a comment

Choose a reason for hiding this comment

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

Thank you @phou87

@chasestarr chasestarr merged commit 7fc8035 into uber:master Sep 2, 2021
chasestarr added a commit that referenced this pull request Sep 3, 2021
khudiono-uber pushed a commit that referenced this pull request Sep 9, 2021
* fix(layout-grid): use correct values for margins vs gutters

* test(vrt): update visual snapshots for f154094 [skip ci]

Co-authored-by: UberOpenSourceBot <UberOpenSourceBot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants