-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Added support for negative offset columns with responsive support #639
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some specific questions. Overall I want to make sure we definitely need all of these before they get added, since it will add a lot of CSS.
This comment has been minimized.
This comment has been minimized.
…s sass to use loops instead of hardcoded classes
@shawnbot I removed a bunch of those classes as we didn't need the full 1-12 spacing range so that list should be different now. Should just cover 1-7 |
Okay @trosage, I got this caught up with v12 and a recent fix to the selector diff report script, and this is what I'm seeing now: > .offset-n1
> .offset-n2
> .offset-n3
> .offset-n4
> .offset-n5
> .offset-n6
> .offset-n7
> .offset-sm-n1
> .offset-sm-n2
> .offset-sm-n3
> .offset-sm-n4
> .offset-sm-n5
> .offset-sm-n6
> .offset-sm-n7
> .offset-md-n1
> .offset-md-n2
> .offset-md-n3
> .offset-md-n4
> .offset-md-n5
> .offset-md-n6
> .offset-md-n7
> .offset-lg-n1
> .offset-lg-n2
> .offset-lg-n3
> .offset-lg-n4
> .offset-lg-n5
> .offset-lg-n6
> .offset-lg-n7
> .offset-xl-n1
> .offset-xl-n2
> .offset-xl-n3
> .offset-xl-n4
> .offset-xl-n5
> .offset-xl-n6
> .offset-xl-n7
> .offset-n1
> .offset-n2
> .offset-n3
> .offset-n4
> .offset-n5
> .offset-n6
> .offset-n7
> .offset-sm-n1
> .offset-sm-n2
> .offset-sm-n3
> .offset-sm-n4
> .offset-sm-n5
> .offset-sm-n6
> .offset-sm-n7
> .offset-md-n1
> .offset-md-n2
> .offset-md-n3
> .offset-md-n4
> .offset-md-n5
> .offset-md-n6
> .offset-md-n7
> .offset-lg-n1
> .offset-lg-n2
> .offset-lg-n3
> .offset-lg-n4
> .offset-lg-n5
> .offset-lg-n6
> .offset-lg-n7
> .offset-xl-n1
> .offset-xl-n2
> .offset-xl-n3
> .offset-xl-n4
> .offset-xl-n5
> .offset-xl-n6
> .offset-xl-n7 Look good? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good except for the comment I left inline!
src/marketing/utilities/layout.scss
Outdated
} | ||
} | ||
|
||
@each $breakpoint, $variant in $responsive-variants { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is listed twice (L22-L29
= L31-37
) accidentally?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch; fixed! ✌️
This PR adds support for negative offset columns. This feature is utilized for marketing purposes with offset grids. This also includes responsive support.
/cc @primer/ds-core