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: use the right breakpoints in block-grid for gutter calculation #11145

Merged
merged 4 commits into from Apr 10, 2018
Merged

fix: use the right breakpoints in block-grid for gutter calculation #11145

merged 4 commits into from Apr 10, 2018

Conversation

DanielRuf
Copy link
Contributor

8a0ae19 produced a regression in the block-grids as this changed the mixin usage and order of arguments.

Also the margin grid part did not use the right breakpoint and so the calculation of the widths was wrong after the mentioned commit as now the breakpoint argument was passed through.

Closes #11121

Copy link
Contributor

@ncoden ncoden left a comment

Choose a reason for hiding this comment

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

I may be wrong as I don't undestand the bug, but I think there is changes in this PR with no impact and unrelated to the issuse it try to solve.

@@ -28,6 +28,6 @@
$size: percentage(1/$n);

& > #{$selector} {
@include xy-cell($size, $gutter-output, $gutters, $gutter-type, $gutter-position, $breakpoint, $vertical);
@include xy-cell($size, $gutter-output: $gutter-output, $gutters: $gutters, $gutter-type: $gutter-type, $gutter-position: $gutter-position, $breakpoint: $breakpoint, $vertical: $vertical);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand. The xy-cell mixin now is:

@mixin xy-cell(
  $size: full,
  $gutter-output: true,
  $gutters: $grid-margin-gutters,
  $gutter-type: margin,
  $gutter-position: right left,
  $breakpoint: null,
  $vertical: false
) {

Why using named parameters here ? Isn't the order right ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Documentation for xy-grid-layout is out-of-date. $breakpoint description is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The named params were set by kball, I will choose the short form here.

Copy link
Contributor

Choose a reason for hiding this comment

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

No I mean:

-  @include xy-cell($size, $gutter-output, $gutters, $gutter-type, $gutter-position, $breakpoint, $vertical); 

is the same as

+  @include xy-cell($size, $gutter-output: $gutter-output, $gutters: $gutters, $gutter-type: $gutter-type, $gutter-position: $gutter-position, $breakpoint: $breakpoint, $vertical: $vertical); 

Why changing ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Named params should be more clear here.

Ah, same for here ?

Copy link
Contributor

@ncoden ncoden Apr 10, 2018

Choose a reason for hiding this comment

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

as there is gutters in between

You are passing $gutters. You pass all arguments with the same name in the same order to xy-cell.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are still referring to the wrong code which is not in the changeset anymore afaik ;-) The change there was reverted.

AI am speaking about https://github.com/zurb/foundation-sites/pull/11145/files#diff-31c7029afafde5d1c8f61e2705198579R191

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please check the current changeset here https://github.com/zurb/foundation-sites/pull/11145/files ;-)

I think there is some miscommunication.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please check the current changeset here https://github.com/zurb/foundation-sites/pull/11145/files ;-)

Yes I seen that. But you seemed to still argue that these are not equivalent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then it was a fault on my side, sorry.

@@ -188,7 +188,7 @@
@include -zf-each-breakpoint {
@for $i from 1 through $xy-block-grid-max {
.#{$-zf-size}-up-#{$i} {
@include xy-grid-layout($i, '.cell', false, $gutter-type: padding);
@include xy-grid-layout($n: $i, $selector: '.cell', $gutter-output: false, $gutter-type: padding, $breakpoint: $-zf-size);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same. xy-grid-layout takes as arguments:

$n,
$selector: '.cell',
$gutter-output: true,

$i, '.cell', false seems to be the right order.

Copy link
Contributor

Choose a reason for hiding this comment

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

A better solution for $breakpoint: $-zf-size would be to make $breakpoint using null by default in @mixin xy-grid-layout, which mean "I will use $-zf-size if it exists, $-zf-zero-breakpoint otherwhise" as defined in @mixin xy-cell.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Named params should be more clear here.

Copy link
Contributor

Choose a reason for hiding this comment

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

So we keep $breakpoint: $-zf-size because "Named params should be more clear here" ?

Please at least change $breakpoint: $-zf-zero-breakpoint to $breakpoint: null in xy-grid-layout and add the missing argument documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Feedback is welcome.

Would be great to get some more feedback from the others.

Copy link
Contributor

@ncoden ncoden left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the changes.

@ncoden ncoden merged commit 9aef331 into foundation:develop Apr 10, 2018
@DanielRuf DanielRuf deleted the fix/block-grid-breakpoints-margins-11121 branch April 10, 2018 22:31
@ncoden ncoden added this to the 6.5.0 milestone Apr 24, 2018
ncoden pushed a commit to ncoden/foundation-sites that referenced this pull request Jun 16, 2018
…points-margins-11121 for v6.5.0

6163a03 fix: use the right breakpoints in block-grid for gutter calculation
0f6659c chore: use short named mixin parameters
8febe20 chore: revert settings file
f542435 chore: document $breakpoint variable in xy-grid-layout, set null as default for $breakpoint

Co-Authored-By: Daniel Ruf <[email protected]>
Signed-off-by: Nicolas Coden <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants