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

New Components - Cards (alternative version) #9215

Merged
merged 8 commits into from Nov 3, 2016
Merged

New Components - Cards (alternative version) #9215

merged 8 commits into from Nov 3, 2016

Conversation

brettsmason
Copy link
Contributor

This is my version of cards for Foundation following #8722 as I felt it needed to be less opinionated and more of a foundation.

Its based on the original version that was in the alpha version of Foundation 6 but some with changes make it cleaner and hopefully more flexible.

Would love some feedback on this!
@kball sorry I ran out of time to do this yesterday like I promised.

This file has all the base styles for cards.
Add the card.md file to the docs to explain and showcase the new component.
Add the card component to the main foundation.scss file for inclusion.
Make sure cards appear in the docs navigation
@brettsmason brettsmason changed the title New Components - Cards New Components - Cards (alternative version) Sep 29, 2016
@DaSchTour
Copy link
Contributor

The most important feature why I originally created the feature component is missing.
Like I mentioned in #8722 and #9208 I wanted to have cards with equal height without javascript by using flexbox direction column.

Imagine cards with the following content.

  1. A header on top
  2. A content section with text of different length
  3. A footer with for some metadata to the content

So now I want that all cards in one row have the header and the footer at the same height. So the content section to fill the space.

With my solution cards can be seamlessly integrated into the existing grid adding the vertical layout that has to be added with javascript in a non flexbox environment.

I have the impression, that both pull requests go into completely different directions.
Mine is about improving vertical layout and this is about adding a header and border to a content element. I think it would have been better to sync the work. I could have remove some features or make them optional in my PR to make it more of a foundation instead of having two different PR that are completely incompatible and none of them implements a satisfying solution.

@brettsmason
Copy link
Contributor Author

@DaSchTour I get what you are saying, and I'm happy to work together to get a solution that everyone is happy with and is useful for a wide range of people.

What do you think the best approach would be to get this into 1 coherent PR that everyone's happy with?

Also would solving #9208 go a long way to help with your vertical card requirement?

@DaSchTour
Copy link
Contributor

Well maybe solving #9208 would create the foundation for vertical layouting in cards. Maybe only a few additions to the grid are needed to create vertical layouting. I'll have to figure this out in the next days.

@zurbrandon
Copy link
Contributor

Hey guys @kball asked me to hop in and check this out.

First off let me add that when we first started looking at Foundation 6 we wanted to strip components out that we found our users used rarely which led us to removing a few components that had been in Foundation for years. The beauty of that is it also made Foundation lighter by default which became a key feature of the new release and drove some awesome initiatives on dropping as much code as possible.

We experimented with cards early on, but choose to remove them on this mantra, but after 8+ months of using the framework on tons of client projects we're totally finding the original need we had for them. They're a component I use on about 50% of projects which definitely meets the qualifications of the framework base.

I'll take a look at this PR with the lens that a very base use of a card would be helpful and see how tiny we can keep it. @DaSchTour I love the idea of using Flexbox for this height reason, but wouldn't want it in the normal implementation of the component. We have however been working on "Flexbox Modes" of every Foundation component which gives you added features with the same mark-up. I'd love to see your implementation added in as a Flexbox mode similar to how we did the top-bar.

I'll check this out today and give any feedback.

Thanks,
B

@zurbrandon
Copy link
Contributor

Checked it out, and it feels nice and simple and looks like you even had some flexbox mode in there which is nice.

@kball I'm ok with this, but lets get some more eyes on it.

Great work @brettsmason!

border: $border;
margin-bottom: $margin;
background: $background;
overflow: hidden;
Copy link
Contributor

Choose a reason for hiding this comment

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

What the purpose of this, i fear it will confuse people trying to position labels that overflow outside the bounds of the box?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zurbrandon so it was in the initial Zurb version of cards too. I think the reasoning is if you wanted to add a border radius to the container (which I guess is quite common) you dont have to apply radius to the top/bottom section and can just apply to the whole container.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yeah, ok. I may have been the one to do that even ha.

Sounds good to me :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zurbrandon Cool thanks for the feedback. One more thing - what are your thoughts on border radius and box shadow as options? They were part of the initial Zurb version but I removed them to tidy it all up. Now I'm thinking they should be options (border radius defaults to global radius, box shadow off by default). That would give the mixin a bit more power.

Or... do you think thats over complicating things?

@brettsmason
Copy link
Contributor Author

@DaSchTour Is there anything else that needs adding to this from your point of view for flexbox support?
I notice flex-direction: column; that I missed from your PR - if you can explain the flexbox requirements a bit we can get it setup so it works for everyones requirements.

…adow.

Added `flex-direction: column` to the container.
@andycochran
Copy link
Contributor

This is rad, @brettsmason. Are you sure that .card-divider is semantic? It doesn't necessarily divide anything. It's simply an alternate background color, right? Maybe .card-content-alt would be better?

@brettsmason
Copy link
Contributor Author

@andycochran So its based on the initial card version the Zurb guys put together - I've also noticed its the same as in Foundation for Apps: http://foundation.zurb.com/apps/docs/#!/card

I guess it will mainly be used for a header/footer to a card. I'm not sure divider is the right name, but I'm not sure what else would fit - I dont feel like .card-content-alt really rolls off the tongue 😄

I was also thinking of changing the .card-content to .card-section like in Apps - what do you think?

@andycochran
Copy link
Contributor

andycochran commented Oct 16, 2016

I like .card-section more than .card-content (the image is content too).

I agree, .card-content-alt isn't perfect. But it's semantic. What about using the whole $foundation-palette — e.g. .card-section.secondary?

… Foundation for Apps version and is more semantic.
@kball
Copy link
Contributor

kball commented Oct 31, 2016

@andycochran @DaSchTour @brettsmason @ncoden What is your sense on the state of this PR?

@DaSchTour I think we're going to move forward with this implementation, rather than #8722, but thank you for getting the ball rolling, and I do want to make sure it handles the flexbox cases you have in mind. Can you take a look and verify? Thanks!

@@ -53,6 +53,7 @@
<li class="docs-nav-title">Containers</li>
<li{{#ifpage 'accordion'}} class="current"{{/ifpage}}><a href="accordion.html">Accordion <span class="label">JS</span></a></li>
<li{{#ifpage 'callout'}} class="current"{{/ifpage}}><a href="callout.html">Callout</a></li>
<li{{#ifpage 'callout'}} class="current"{{/ifpage}}><a href="card.html">Card</a></li>
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this was copied from the previous line. Shouldn't #ifpage 'callout' be #ifpage 'card'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@andycochran Thanks for pointing that out, had missed that! 😄
I've just corrected it.

@brettsmason
Copy link
Contributor Author

@kball from my point of view it should be about ready, but others testing it too would be good.

@andycochran
Copy link
Contributor

Looks god to me. @brettsmason is a champion!
@kball I say 👍 so long as Travis just needs a kick.

@kball
Copy link
Contributor

kball commented Oct 31, 2016

There are travis issues that are related to the version this was branched off of, so I checked it out to run with updated tests, and ran into this issue building:

Error in plugin 'sass'
Message:
    scss/components/_card.scss
Error: required parameters must precede optional parameters
        on line 54 of scss/components/_card.scss
>>   $shadow: $card-shadow,
   -----------------------^

@brettsmason
Copy link
Contributor Author

@kball Sorry I missed this. I'll try and sort this tonight. I'll also run some tests just to make sure all is working properly before we merge.

@DaSchTour
Copy link
Contributor

@kball no it doesn't handle the flexbox cases I had in mind. My implementation based on the idea that a card might have a header and footer see .card-header and .card-footer in my PR and a .card-block with flex-grow in the middle. flex-grow in this cases makes the block and only the block take all space that is available. I also made a slight different implementation here: https://github.com/DaSchTour/foundation-components/blob/master/scss/_cards.scss
I currently don't have the time to verify if this implementation does it right. But from a first look I don't think so.

@kball
Copy link
Contributor

kball commented Nov 2, 2016

OK @DaSchTour I think I understand the case you're tackling enough to take a pass on it. @brettsmason so that I understand... what was the reason for making dividers have the same flex properties as sections? It seems like what might behave a bit better (and possibly address @DaSchTour 's concern) is to have something like

.card-section {
  flex: 1 0 auto;
}

.card-divider {
  flex: 0 1 auto;
}

So that dividers shrink down to minimum necessary height by default and the sections take up the remainder

@kball kball merged commit 91c6a92 into foundation:v6.3 Nov 3, 2016
@kball
Copy link
Contributor

kball commented Nov 3, 2016

Great work @brettsmason!

@andycochran
Copy link
Contributor

:standing-ovation:

@DaSchTour
Copy link
Contributor

Well, I checked 6.3.0-rc.1 to verify that what I created with my PR works and it doesn't. Well it does but not the way that it looks good and as it is documented.
This is the nearest I get to what I wanted to achieve.
http://codepen.io/DaSch/pen/wopRGN
To have the footer at the bottom the columns need to be cards, but that make the border is misplaced away. I wish we would have build upon my solution. The current flexbox style doesn't change anything so it can be removed.

@brettsmason
Copy link
Contributor Author

@DaSchTour I'm sorry you feel that way - the intention was never to disregard your PR or your suggestions, I just think a simpler card implementation was desired, and add more utility outside of cards that enabled what you wanted to achieve.

@kball added a PR which contains a bunch of flexbox utility mixins/classes. The idea was to provide this sort of functionality to everything, rather than just cards. You can see the PR here: #9324

I had a look at your Codepen and modified it, take a look here: http://codepen.io/brettsmason/pen/GNyPOw

Is that what you were trying to do?

@DaSchTour
Copy link
Contributor

@brettsmason yes that's exactly what I achieved with my card implementation. I just don't really see the point why it now needs that many additional classes. I hoped there could be a more simple solution.

@brettsmason
Copy link
Contributor Author

@DaSchTour There's always mixins if you wanted to create something simpler to implement? Create your own class that has all relevant mixins assigned and you should be good to go.

@DaSchTour
Copy link
Contributor

@brettsmason well sure. I'm already heavily make use of foundations flexbox mixins. Although naming get's really confusion if changing the direction. Right equals bottom and Left equals top and so on. If you work with flexbox a lot it's simple to layout what ever you want from scratch. I thought I could provide a simple solution for a common problem that can be used by everybody without diving deep into flexbox.

@brettsmason
Copy link
Contributor Author

brettsmason commented Dec 2, 2016

@DaSchTour So looking at this some more, there's just a few pieces of the puzzle missing from what I understand about flexbox.

With all the classes @kball added, we could use one that sets all the children as display: flex; correct?
Something like this:

@mixin flex-grow-all(
$children: '.column') {
  #{$children} {
    display: flex;
  }
}

.flex-grow-all {
  @include flex-grow-all();
}

Then all that needs adding to .card is flex-grow: 1; right?

Alternatively you add this as a .card-row class as you suggested, which adds that, plus the needed class on .card as a new function, but I would prefer a re-usable component if possible.

I'd like to get this usable for you so you are happy with it, so if you can work with me on a solution I'd appreciate it. Codepen: http://codepen.io/brettsmason/pen/GNyPOw

@rafibomb
Copy link
Member

rafibomb commented Dec 2, 2016

@brettsmason Loving this new pattern! I started making a card library with them and so far so good! You are right that one way to handle this is with flex utility classes. Another way, what Daniel is suggesting is to use the flex grow property to automatically do this. With flexbox, the columns in a row are the same height, so getting the card divider to sit on the bottom totally work.

@DaSchTour Love your idea! I think that it makes sense that a .card-divider at the bottom of the card should be aligned: bottom.

The difference:
Daniels version has cards attached to the columns. This presents a challenge which spacing between the cards.

screen shot 2016-12-02 at 10 01 43 am

vs

screen shot 2016-12-02 at 10 05 35 am

There is probably another way to handle this where we set the .align-stretch on the containing row and the .card-section can be set to grow.

@DaSchTour What do you think? Can we get a flex implementation that blends the your technique in with the current version where the card is inside the column rather than on it?

@rafibomb
Copy link
Member

rafibomb commented Dec 2, 2016

@DaSchTour @brettsmason

Ok so played around with it more - I think we can achieve both easily.

Columns are not display: flex by default (various reasons) but if we add a class to Foundation that makes them display flex, the card (or any content inside) will be full height.

screen shot 2016-12-02 at 10 21 16 am

How about suggesting a .flex-column helper class to "stretch" the card?

@andycochran
Copy link
Contributor

As @ncoden might say… 😉
We should keep the .column-[modifier] namespace. Maybe the class should be .column-flex?

@brettsmason
Copy link
Contributor Author

@andycochran yep that sounds good. What about a class that applies it to all children too? In this case you would apply it to the row. Or even an option with block grid?

@rafibomb
Copy link
Member

rafibomb commented Dec 2, 2016

ooo interesting!

@andycochran right on .column-flex

@brettsmason Hmm, yeah it would make sense, what to name it thought?

@brettsmason
Copy link
Contributor Author

@rafibomb not sure, would need to look through the new flex helper classes. I wonder if a lot of those new classes could also have a version that applies the effect to all children instead?

@ncoden
Copy link
Contributor

ncoden commented Dec 3, 2016

What about a class that applies it to all children too?

@brettsmason This seems to be the same concept as in: #9098 (comment). You could use .row-flex and .column-flex.


According to me, the card is a graphical component and the grid is a structural component. Columns that take the full height should be the grid job, so I don't like the flex properties applied to .card and its elements.

Maybe you could use row and column modifiers to apply flex properties inside a card ? So you can create non-card component with the "grow" behavior, or card components without it, simply by using or not .card component and .row/.column elements and modifiers in your markup.

@rafibomb @DaSchTour With that, you could can have cards with or without margins by applying using the card component inside columns (.column .card) or on it (.column.card).

What do you think ?

@paxperscientiam
Copy link
Contributor

paxperscientiam commented Dec 16, 2016

According to me, the card is a graphical component and the grid is a structural component.

I think this is a really important discussion to have and the answer, I believe, hinges on design goals with presently undecided levels of priority.

These are decided:

  • Mobile, desktop, or tablet first? MOBILE FIRST

  • Designer (font-end) or developer first (back-end)? DESIGNER FIRST

These are not settled (at least not consistently):

  • Desired learning curve? Power or normal user focused?

  • More weight to structure or more weight to content?

Here's a relevant example: If I'm a noobie checking out Foundation for the first time, I might check out this card example and, perhaps, conclude that card heights will match no matter the content provided I use the row-grid structure. This, isn't the case.

While chaining card blocks yields matching height cards within a row block, nesting each card in a column prevents this.

The linked approach makes it easier to maintain common heights; the second approach places content one level deeper to make it easier to control column widths.

The correct choice, naturally, depends on the design philosophy of Zurb. Personally, I like the idea of keeping markup as simple as possible. @brettsmason made the markup for Off-Canvas so much simpler -- it's elegant.

IMHO, pushing for greater markup elegance will make Foundation more accessible for users who rely on pre-built Foundation. Leveraging @zurbrandon 's work on frame-grid would help a lot.

¢¢

@andycochran
Copy link
Contributor

andycochran commented Dec 16, 2016

What if the "chained cards" approach had an optional .card-group wrapper element which extended block grid styles, allowing its cards to behave as columns? This is tricky 'cause the grid uses padding for gutter, but this "grid" could use margin.

@ncoden
Copy link
Contributor

ncoden commented Dec 17, 2016

@andycochran We have the grids to create complete and almost fully customizable rows and columns. Why re-create everything, the entirely structural logic, instead of using the grid's one ?

@andycochran
Copy link
Contributor

Why do we have button group when we could just put expanded buttons in a collapsed grid?

@ncoden
Copy link
Contributor

ncoden commented Dec 17, 2016

Good question. 😄

More seriously, because -for the specific case of button groups- :

  • The collapsed grid markup is unnecessary complicated for this case.
  • The semantic of a one-column collapsed grid have nothing to do with a set of inline items.
  • The structural logic to recreate is very simple.

I think this is the reasons why we do not use the collapsed grid for buttons. But maybe we should, I don't know the collapsed buttons behaviour to be sure about that. Is there others reasons ?

However, I know the reasons I given does not apply to Cards. Cards are not simple components, we expect from them a complex and customizable behavior that is very close to the grid one, with a similar markup.

@andycochran
Copy link
Contributor

andycochran commented Dec 17, 2016

This is rough, but totally works.

$card-group-max: 6 !default;

.card-group {
  @include clearfix;
}

@include -zf-each-breakpoint {
  @for $i from 1 through $card-group-max {
    .card-group-#{$-zf-size}-up-#{$i} {
      @include grid-layout($i, '.card', 0);
      margin-right: ($card-margin * ($i - 1));

      .card:not(:nth-child(#{$i}n+#{$i})) {
        margin-right: $card-margin;
      }
      .card:nth-child(#{$i}n+#{$i}) {
          margin-right: -$card-margin * $i;
      }
    }
  }
}
<div class="card-group card-group-medium-up-4">
  <div class="card">
    <div class="card-section">
      <p>A card in a grid.</p>
    </div>
  </div>
  <div class="card">
    <div class="card-section">
      <p>A card in a grid.</p>
    </div>
  </div>
  <div class="card">
    <div class="card-section">
      <p>A card in a grid.</p>
    </div>
  </div>
  <div class="card">
    <div class="card-section">
      <p>A card in a grid.</p>
    </div>
  </div>
</div>

@ncoden
Copy link
Contributor

ncoden commented Dec 17, 2016

This is rough

You are right. How many CSS lines does it generate ?
And it does not provide a way to add or remove margins between cards, isn't it ?

@brettsmason
Copy link
Contributor Author

Could we not set a negative margin on either side of the container and then apply margin both sides to the card? Might be a bit simpler possibly?

@paxperscientiam
Copy link
Contributor

@andycochran Might I point out something? Semantics aside, cards can fit into current (flex) grid system in the following way:

<div class="row">
  <div class="card small-12 medium-4">
<div class="card-section">
    <p>Card Uno</p>
   </div>
</div>
 <div class="card small-12 medium-4">
<div class="card-section">
    <p>Card Dos</p>
   </div>
</div>
 <div class="card small-12 medium-4">
<div class="card-section">
    <p>Card Tres</p>
   </div>
</div>

You just have to be comfortable with dropping ".column" from the markup.

@andycochran
Copy link
Contributor

Then all we'd need is…

.card-group {
  @include grid-row;
}

…which could add zero new lines of code.

@paxperscientiam
Copy link
Contributor

paxperscientiam commented Dec 19, 2016

@andycochran Righ, so .card-group becomes a synonym for .row and .card-group-[size]-up-$[n] a synonym for .[size]-up-[n]

EDIT:
Did you mean @include flex-grid-row? That is what worked for me.

EDIT 2:
Here is a visual test I started that includes .card-group, though not yet .card-group-[size]-up-$[n].
linky

@ncoden
Copy link
Contributor

ncoden commented Dec 19, 2016

@andycochran @paxperscientiam

@include grid-row; add a lot of code.

Can you talk about it in a new issue ?
And please, try to keep things simple. Do not include whole mixins to have an alias, "add" features that already exist, or add "do-everything" components while we could just let the user build what he want with bricks.

@andycochran Do you have others remarks about #9215 (comment) ?

Thank you. 😓

@rafibomb
Copy link
Member

rafibomb commented Dec 20, 2016

Re: cards without spacing or margins -
@paxperscientiam You can still achieve that layout within .columns if you add the .collapse class on the row. http://codepen.io/rafibomb/pen/ZBwZLY

Re: matching height cards - @DaSchTour
Matching height cards can be achieved with the new helper class on the column, .flex-container.
http://codepen.io/rafibomb/pen/rWPbmO

I opened a new issue where we can focus features of cards on. If I'm missing something there, please add a comment on issue #9533

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.

None yet

8 participants