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

Adds breakpoints to sizing utility classes #24015

Closed
wants to merge 1 commit into from

Conversation

andresgalante
Copy link
Collaborator

@andresgalante andresgalante commented Sep 20, 2017

This PR closes #24003 and it adds breakpoints to sizing utility classes.

It also updates the docs to surface this change.

@Johann-S you know my feelings about utility classes 💩 but I think that since we are doing this for spacers, adding it for sizing is valid.

Closes #24062 too

@Herst
Copy link
Contributor

Herst commented Sep 22, 2017

While at it wouldn't printing-specific versions also be useful?

@andresgalante
Copy link
Collaborator Author

@Herst yeap, it would make sense. Help me out a bit, what do you have in mind for printing?

A question I keep asking myself when doing utility classes is how far do we want to take them, and when to stop.

@Herst
Copy link
Contributor

Herst commented Sep 22, 2017

@andresgalante See for example: https://stackoverflow.com/a/32253969

@andresgalante
Copy link
Collaborator Author

@Herst yeah, I see what you mean, thanks for the clarification. Should we include it on this PR or track it in another one?

@Herst
Copy link
Contributor

Herst commented Sep 22, 2017

I separate PR makes much more sense, especially now that this one here has been reviewed. Sorry for hijacking it (I was sort-of thinking aloud).

@andresgalante
Copy link
Collaborator Author

Closes #24062 too

@XhmikosR
Copy link
Member

@Johann-S: looks good to you?

@Johann-S
Copy link
Member

Not against it but @mdo have concerns about our CSS file size, so I'll leave the end word to Mark here

@andresgalante
Copy link
Collaborator Author

@XhmikosR thanks for reviewing it. I agree with @Johann-S, this is a "nice to have" to follow other utility classes, but I wouldn't mind if it doesn't get merged. it's not fixing any bug, just adding more options. So, it's more a matter of if we want this on Bootstrap or not.

On the other hand I have other open PRs that are fixing issues that we should merge.

@mdo
Copy link
Member

mdo commented Sep 26, 2017

I'd rather hold off on this one—I want to verify these utilities get used much in the first place before we 5x the number of them by making them responsive :).

@Herst
Copy link
Contributor

Herst commented Sep 26, 2017

@mdo Same concerning the idea with the printing classes I guess?

@mdo
Copy link
Member

mdo commented Oct 2, 2017

Print-specific classes are probably fine? Depends on the situation. Open an issue to discuss?


@each $prop, $abbrev in (width: w, height: h) {
@each $size, $length in $sizes {
.#{$abbrev}-#{$size} { #{$prop}: $length !important; }
Copy link
Contributor

@iamandrewluca iamandrewluca Oct 20, 2017

Choose a reason for hiding this comment

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

@andresgalante it seems this line is not needed
when $breakpoint is xs, $infix is an empty string
This line wil catch the case

.#{$abbrev}#{$infix}-#{$size} { #{$prop}: $length !important; }

Copy link
Contributor

@iamandrewluca iamandrewluca Oct 20, 2017

Choose a reason for hiding this comment

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

Adding sizing for remaining breakpoints.

// Create sizing classes for all screen sizes
@each $breakpoint in map-keys($grid-breakpoints) {
  $infix: breakpoint-infix($breakpoint, $grid-breakpoints);

  @if $infix != '' {
    @include media-breakpoint-up($breakpoint) {
      @each $prop, $abbrev in (width: w, height: h) {
        @each $size, $length in $sizes {
          .#{$abbrev}#{$infix}-#{$size} { #{$prop}: $length !important; }
        }
      }
    }
  }
}

Copy link
Member

Choose a reason for hiding this comment

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

@iamandrewluca: please make a PR; commenting in older PRs/issues will probably be ignored.

Copy link
Contributor

Choose a reason for hiding this comment

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

@XhmikosR I just wanted to mention this to @andresgalante . As I see this is not being merged. Should I create a PR in any case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@iamandrewluca you are right, thanks a lot for reviewing it. 😄

@XhmikosR
Copy link
Member

XhmikosR commented Mar 22, 2018

I'd still like to have this merged.

It seriously improves usage.

@mdo: are you still opposed to this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants