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

Remove the card columns in favor of a Masonry grid #28922

Merged
merged 7 commits into from Jul 22, 2019

Conversation

MartijnCuppens
Copy link
Member

@MartijnCuppens MartijnCuppens commented Jun 18, 2019

Mentioned before in #28891 (comment) and #28076 (comment), we should ditch the card columns in favor of masonry.

Main reasons are:

I've updated the docs and included Masonry via cdnjs, @XhmikosR is that ok for you, or do you have another proposal? (I also watched the releases of masonry github repo so I know when to upgrade)

Edit:
The demo is a example page now. It's also possible to add additional parameters (async & intergrity) to the extra js.

TODO:

  • Link to this PR in migration section
  • Move documentation to examples
  • Allow to pass async attribute
  • Check if integrity attribute is present before printing

Closes #28076, closes #28439

Card docs: https://deploy-preview-28922--twbs-bootstrap.netlify.com/docs/4.3/components/card/#card-columns-masonry-layout
Example page: https://deploy-preview-28922--twbs-bootstrap.netlify.com/docs/4.3/examples/#integrations
Demo: https://deploy-preview-28922--twbs-bootstrap.netlify.com/docs/4.3/examples/masonry/

@MartijnCuppens MartijnCuppens added this to Inbox in v5 via automation Jun 18, 2019
@MartijnCuppens MartijnCuppens marked this pull request as ready for review June 18, 2019 07:34
@MartijnCuppens MartijnCuppens requested a review from a team as a code owner June 18, 2019 07:34
ysds
ysds previously approved these changes Jun 18, 2019
Copy link
Member

@ysds ysds left a comment

Choose a reason for hiding this comment

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

LGTM 👍, and @mdo?

v5 automation moved this from Inbox to Approved Jun 18, 2019
@mdo
Copy link
Member

mdo commented Jun 18, 2019

Thoughts on punting the inclusion of external JS to an example instead of a core component page? We can link to it from the Card docs, but I'd prefer to build something there with Masonry instead of adding it to the core docs.

@mdo
Copy link
Member

mdo commented Jun 18, 2019

But also, yes, definitely down to ditch the card columns lol.

@MartijnCuppens
Copy link
Member Author

Examples might be a better place indeed, I'll change it

@inwardmovement
Copy link
Contributor

inwardmovement commented Jun 27, 2019

Maybe we should simply use native css subgrids to prevent having to use a "card-deck" container or a plugin:
Hello Subgrid! – Rachel Andrew at CSSconf EU 2019

Not supported everywhere (anywhere?) but should be since next Bootstrap version I guess.

@MartijnCuppens
Copy link
Member Author

@inwardmovement, we'll probably introduce grid in Bootstrap 6 (the next version(5) will still have IE support), but the masonry layout still differs from (sub)gird.

@MartijnCuppens MartijnCuppens dismissed ysds’s stale review June 28, 2019 08:31

Dismissing review since we're moving the demo to the example section

@MartijnCuppens MartijnCuppens force-pushed the master-mc-masonry-card-layout branch 3 times, most recently from 93afb9b to 9e5c05d Compare June 30, 2019 14:28
@MartijnCuppens MartijnCuppens moved this from Approved to Review in v5 Jul 1, 2019
@MartijnCuppens
Copy link
Member Author

The masonry example is now moved to the examples section.

@mdo
Copy link
Member

mdo commented Jul 17, 2019

Agreed with @XhmikosR about using the existing Examples layout. That's a little tougher for some of the needs here, but this does feel slightly over-engineered right now. We may end up adding third party JS to more examples, but I don't know when that'd be. Until then, these examples should look and feel isolated from our docs.

Here's what I'm leaning towards after taking a crack at this branch (see latest commit):

Screen Shot 2019-07-17 at 2 40 45 PM

@XhmikosR
Copy link
Member

I'm gonna fix the conflict later but we should remove the copy example because it requires our docs CSS and JS.

@XhmikosR XhmikosR force-pushed the master-mc-masonry-card-layout branch from 90a11e5 to 134a47d Compare July 18, 2019 09:27
v5 automation moved this from Review to Approved Jul 18, 2019
@XhmikosR
Copy link
Member

XhmikosR commented Jul 19, 2019

This needs some more work and especially the part with the hardcoded script is something that I personally don't like. Maybe we should link to their docs?

Also, I need to rebase and revert the active menu change.

@XhmikosR XhmikosR force-pushed the master-mc-masonry-card-layout branch 3 times, most recently from 0a061d4 to 3eb6371 Compare July 19, 2019 15:54
@TinajaLabs
Copy link

Somehow I missed all this and used .card-columns. Now wanting to go from 4.6 to 5.3 and yuck, .card-columns is broken. My view is x number of columns wide, starting at the upper left going down each column as many as are needed for more or less evenly distributed column counts.

There's no getting around the efficiency of having all the cards/images stacked in columns when the images are the same width but different heights. No gaps or wasted screen space; only at the bottom.

Does this mean there is no longer a mechanism for columnar lists?

It seems in masonry, the best alternative is horizontalOrder: true and within a few rows down is impossible to follow in any reasonable visual order. If anyone has tips... Thanks.

@julien-deramond
Copy link
Member

IMO you'll have more chance of getting an answer by opening a new Discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v5
  
Shipped
Development

Successfully merging this pull request may close these issues.

.card-columns causes svg elements inside to not be visible upon resize in Chrome Masonry-ish grid
8 participants