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.

Sign up for GitHub

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

Jump to bottom

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6163a03
fix: use the right breakpoints in block-grid for gutter calculation
DanielRuf Apr 8, 2018
0f6659c
chore: use short named mixin parameters
DanielRuf Apr 9, 2018
8febe20
chore: revert settings file
DanielRuf Apr 9, 2018
f542435
chore: document $breakpoint variable in xy-grid-layout, set null as d…
Apr 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions scss/xy-grid/_classes.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

}
}
}
Expand All @@ -203,15 +203,15 @@
@each $bp in -zf-breakpoints-less-than($-zf-size) {
@if(map-has-key($grid-margin-gutters, $bp)) {
.grid-margin-x.#{$bp}-up-#{$i} {
@include xy-grid-layout($i, '.cell', false, $gutter-type: margin, $breakpoint: $bp);
@include xy-grid-layout($n: $i, $selector: '.cell', $gutter-output: false, $gutter-type: margin, $breakpoint: $-zf-size);
}
}
}
}
}
@for $i from 1 through $xy-block-grid-max {
.grid-margin-x.#{$-zf-size}-up-#{$i} {
@include xy-grid-layout($i, '.cell', false, $gutter-type: margin, $breakpoint: $-zf-size);
@include xy-grid-layout($n: $i, $selector: '.cell', $gutter-output: false, $gutter-type: margin, $breakpoint: $-zf-size);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion scss/xy-grid/_layout.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited
Loading

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.

}
}