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

v4/v5: Row columns for responsive card decks and more #29073

Merged
merged 8 commits into from
Aug 30, 2019
Merged

v4/v5: Row columns for responsive card decks and more #29073

merged 8 commits into from
Aug 30, 2019

Conversation

mdo
Copy link
Member

@mdo mdo commented Jul 17, 2019

Overview

Trying to find a new way to do responsive card decks while not locking ourselves into more card classes. My thinking here is we can easily control the column (.col) width by the parent with responsive .row-cols-* classes, but I don't know how many we need (have 0-5 now).

<!-- Three columns at the medium breakpoint and up -->
<div class="row row-cols-md-3">
  <div class="col">Column</div>
  <div class="col">Column</div>
  <div class="col">Column</div> <!-- Wraps to new line here -->
  <div class="col">Column</div>
  <div class="col">Column</div>
</div>

/cc @twbs/css-review

How it works

We can add a default number of columns to support (thinking a max of 6). You can override this if you like, but otherwise we'll loop over it and generate the responsive classes.

Here's a simplified example with out the responsive infixes. See it in action via Sassmeister.

$row-columns: 6 !default;

@for $i from 1 through $row-columns {
  .row-cols-#{$i} > [class^="col"] {
    flex: 0 0 calc(100% / #{$i});
  }
}

Screenshots

Some screenshots to show how it's shaping up for grid columns and cards.

Screen Shot 2019-07-17 at 4 26 12 PM

Screen Shot 2019-07-17 at 4 26 15 PM

Preview: https://deploy-preview-29073--twbs-bootstrap.netlify.com/docs/4.3/components/card/#grid-cards

https://deploy-preview-29073--twbs-bootstrap.netlify.com/docs/4.3/layout/grid/#row-columns

@mdo mdo mentioned this pull request Jul 18, 2019
@mdo
Copy link
Member Author

mdo commented Jul 18, 2019

And here's how it could work with some Sass mixins: https://www.sassmeister.com/gist/1a567b85b0c7c5a837e9b65be6220688.

@mdo mdo changed the title v5: Row columns for responsive card decks and more v4/v5: Row columns for responsive card decks and more Jul 23, 2019
@mdo mdo added the v4 label Jul 23, 2019
@mdo mdo marked this pull request as ready for review July 24, 2019 06:00
@mdo mdo requested a review from a team as a code owner July 24, 2019 06:00
@mdo
Copy link
Member Author

mdo commented Jul 24, 2019

This is ready for review @twbs/css-review.

@MartijnCuppens
Copy link
Member

MartijnCuppens commented Jul 30, 2019

I don't see the benefit of this. We can just use the grid for this.

@JacobLett
Copy link

I don't see the benefit of this. We can just use the grid for this.

This would be really helpful when you're pulling a dynamic list and unable to change the grid markup each time. Having it on the parent class would make it "set it and forget it".

@JacobLett
Copy link

Could we have these classes responsive with breakpoints that way we can have 1 column stacked on mobile and multiple columns on desktop?

row-cols-1 row-cols-md-3

@mdo
Copy link
Member Author

mdo commented Aug 13, 2019

Could we have these classes responsive with breakpoints that way we can have 1 column stacked on mobile and multiple columns on desktop?

Yup, that's the intent of the changes here: https://github.com/twbs/bootstrap/pull/29073/files#diff-f6ca44772c5cdca97cf5be7357416fb8.

Trying to find a new way to do responsive card decks while not locking ourselves into the cards themselves. My thinking here is we can easily control the column (.col) width by the parent, but I don't know how many we need (have 0-5 now) across each breakpoint. This works for cards so far, and I think could get us equal height, too.
- Rename and move the variable to variables file
- Move code to the grid file
- Use the mixin to generate our own classes
- Wrap in a grid classes enabled conditional
- Document the mixin
@XhmikosR
Copy link
Member

Rebased

@MartijnCuppens
Copy link
Member

Ok, I think this can come in handy in some situations, but the current implementation needs some fine tuning:

  • The [class^="col"] doesn't really make sense. You can use whatever col class you want, but the implementation is always overridden by the .row-cols-* class?
  • We don't use the calc() notation in our grid cols, so it might also be better to use percentage() here (or use calc() in both places)

@MartijnCuppens
Copy link
Member

  • The [class^="col"] doesn't really make sense. You can use whatever col class you want, but the implementation is always overridden by the .row-cols-* class?
  • We don't use the calc() notation in our grid cols, so it might also be better to use percentage() here (or use calc() in both places)

Made an alternative implementation draft of this in #29274 which solves these issues

@XhmikosR
Copy link
Member

Should we close this @mdo in favor of #29274?

@ysds
Copy link
Member

ysds commented Aug 27, 2019

@XhmikosR I want to comment, so please leave this. (but sorry, I don't have time today...)

@ysds ysds mentioned this pull request Aug 28, 2019
@ysds
Copy link
Member

ysds commented Aug 28, 2019

The difference between this PR and #29274 is the need for .col class and whether the .col- class can override the default width.

I support the @mdo proposal which requires .col class. Because, Bootstrap's grid system has the rule:

In a grid layout, content must be placed within columns and only columns may be immediate children of rows.
https://deploy-preview-29073--twbs-bootstrap.netlify.com/docs/4.3/layout/grid/#how-it-works

If this traditional rule changes, it will confuse the many user.

This PR can not overriding the default width because of the CSS priority, but this can be fixed by changing the definition order.

I created #29317 for clarify the diff.

@XhmikosR
Copy link
Member

Is this ready from all of you @twbs/css-review?

@MartijnCuppens
Copy link
Member

Since we (@mdo, @ysds and me) all agreed on #29317, I assume it is

@ysds
Copy link
Member

ysds commented Aug 29, 2019

It is better to add a responsive examples on the grid page :)

@XhmikosR
Copy link
Member

@ysds can you do it please?

@ysds
Copy link
Member

ysds commented Aug 29, 2019

done!

@XhmikosR
Copy link
Member

@mdo please have a final look and hit rebase and merge if it LGTY, thanks!

@mdo mdo merged commit b1f4909 into master Aug 30, 2019
@mdo mdo deleted the row-cols branch August 30, 2019 20:07
@mdo mdo mentioned this pull request Aug 30, 2019
@XhmikosR
Copy link
Member

This needs to be backported manually in my v4-dev-xmr branch.

I'll have another try again later, but it conflicts a lot. Also note that we need to make sure we don't use Hugo-related stuff in v4, like placeholder, docsref and example.

@XhmikosR XhmikosR mentioned this pull request Sep 2, 2019
emanb29 added a commit to emanb29/pdx-cs565-portfolio that referenced this pull request Apr 25, 2020
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.

5 participants