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

Clean up variables, comments, reduce complexity #10430

Merged
merged 4 commits into from Oct 10, 2018

Conversation

Projects
None yet
4 participants
@jasmussen
Contributor

jasmussen commented Oct 9, 2018

This PR does a few things:

  1. It replaces the $item-spacing variable with $grid-size, which is one small step towards a base8 based grid.
  2. It cleans up comments in the variables file, fixes a few typos, and clarifies comments.
  3. It retires a rarely used variable and makes the SCSS math that used it slightly more clear.

Aside from a good code review and testing, this needs a visual regression sanity check. The $grid-size variable is 8px, whereas $item-spacing was 10px. In my own testing, this caused no issues, but it would still be good to check a bit more in-depth. If some margins are now too small, we can consider a 12px variant, or jump directly to 16px.

@jasmussen jasmussen self-assigned this Oct 9, 2018

@jasmussen jasmussen requested review from tofumatt and WordPress/gutenberg-core Oct 9, 2018

@karmatosed

This comment has been minimized.

Show comment
Hide comment
@karmatosed

karmatosed Oct 9, 2018

Member

Design feedback wise 👍 on this.

Member

karmatosed commented Oct 9, 2018

Design feedback wise 👍 on this.

@talldan

talldan approved these changes Oct 10, 2018 edited

I did a side-by-side test and didn't notice anything broken.

I did spot that #9814 seems to have regressed, and things are more pronounced on this branch.

Nothing major though, it still looks good. 👍

jasmussen added some commits Oct 9, 2018

Clean up variables, comments, reduce complexity
This PR does a few things:

1. It replaces the $item-spacing variable with $grid-size, which is one small step towards a base8 based grid.
2. It cleans up comments in the variables file, fixes a few typos, and clarifies comments.
3. It retires a rarely used variable and makes the SCSS math that used it slightly more clear.

Aside from a good code review and testing, this needs a visual regression sanity check. The $grid-size variable is 8px, whereas $item-spacing was 10px. In my own testing, this caused no issues, but it would still be good to check a bit more in-depth. If some margins are now too small, we can consider a 12px variant, or jump directly to 16px.
@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Oct 10, 2018

Contributor

Solid feedback.

I rebased, and fixed the visual regression:

screenshot 2018-10-10 at 10 00 21

Although that aspect is no longer fully base-8 gridbased, this is to be expected. The regression here is that the X is a dashicon (base10 grid), and the chevrons are material icons (base8 grid). So I had to make an exception there to make them align.

Contributor

jasmussen commented Oct 10, 2018

Solid feedback.

I rebased, and fixed the visual regression:

screenshot 2018-10-10 at 10 00 21

Although that aspect is no longer fully base-8 gridbased, this is to be expected. The regression here is that the X is a dashicon (base10 grid), and the chevrons are material icons (base8 grid). So I had to make an exception there to make them align.

@jasmussen jasmussen added this to the 4.1 milestone Oct 10, 2018

@jasmussen jasmussen merged commit c0b9c05 into master Oct 10, 2018

2 checks passed

codecov/project 49.48% (+<.01%) compared to f232447
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasmussen jasmussen deleted the polish/comments branch Oct 10, 2018

@gziolo gziolo modified the milestones: 4.1, 4.0 Oct 10, 2018

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