-
Notifications
You must be signed in to change notification settings - Fork 17
refactor(breakpoints): use same breakpoints for media queries and gri… #360
Conversation
@sambhav-gore great job! Try also to have a look here: https://github.com/zalando/dress-code/blob/master/docs/demo/views/index.html#L32 And in the docs folder in general.... try also to build the demo site because I think there we are still using |
@rbarilani Good catch. I will remove it from docs now. Have already modified demo to use media queries so breakpoint-sass is not even a devDependency. |
@sambhav-gore Is possible to don't introduce breaking changes? |
@include breakpoint($dc-small-width) { | ||
@include dc-debugmessage("Small \2014 320-480px", $dc-orange60); | ||
@media (min-width:$dc-small-width) { | ||
@include dc-debugmessage("Small \2014 320-600px", $dc-orange60); |
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.
Wouldn't this be 599px
instead of 600px
if your next breakpoint has min-width?
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.
@frontendherodk Good catch, I will recheck in the demo and correct accordingly.
// Feature phone: 0px | ||
$dc-tiny-width: 0 !default; | ||
/// This bloats the CSS use only those which you need | ||
/// |
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.
Do you want to include tiny here?
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.
@frontendherodk Yes, same as Giant and Monstrous width, this should be added back. Is part of my future commit.
Just want to say, finally! Thx Sam! |
@rbarilani @frontendherodk I have addressed the review comments and also tested it locally. |
@sambhav-gore I will rebase (squash) everything in one commit and be sure to have a good commit message... especially the breaking change part.... for what I got: * $dc-large-width breakpoint variable renamed to $dc-medium-width
* $dc-huge-width breakpoint variable renamed to $dc-large-width
* $dc-huge-width breakpoint variable renamed to $dc-giant-width
* $dc-giant-width and $dc-monstruous-width are deprecated and not used anymore |
@rbarilani +1 for squashing. 3rd point in your comment is not correct. it is other way Giant is renamed to large, (same as 80em).
|
@sambhav-gore Yep I got confused.... anyway you got the point.... right? ;) |
@rbarilani yes. |
…d classes remove breakpoint-sass as dependency BREAKING CHANGE: * $dc-large-width breakpoint variable renamed to $dc-medium-width * $dc-huge-width breakpoint variable renamed to $dc-large-width * $dc-giant-width breakpoint variable renamed to $dc-huge-width * $dc-giant-width and $dc-monstrous-width are deprecated and not used anymore Closes #239 chore(bower): drop bower support (#357) close #346 chore(dist): remove dist from version control (#358) close #347 docs(usage): update usage; remove breakpoint-sass dependency docs(breakpoints): correct sizes in breakpoint width display
72065eb
to
7ee7273
Compare
👍 |
1 similar comment
👍 |
…d classes
remove breakpoint-sass as dependency.
This is a breaking change for anyone directly using the sass variables.
Closes #239