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

Card tables #25193

Closed
wants to merge 4 commits into from
Closed

Card tables #25193

wants to merge 4 commits into from

Conversation

MartijnCuppens
Copy link
Member

@MartijnCuppens MartijnCuppens commented Jan 3, 2018

mdo
mdo previously approved these changes Jan 21, 2018
@ysds
Copy link
Member

ysds commented Jan 21, 2018

This feature will break a table including merged (rowspan) cells. Probably there is no reasonable way to solve this. Are you sure you want to add this?

<table class="table table-bordered">
  <thead>
    <tr><th>Name</th><th>Data</th></tr>
  </thead>
  <tbody>
    <tr><td rowspan="3">Name 01</td><td>001</td></tr>
    <tr><td>002</td></tr>
    <tr><td>003</td></tr>
    <tr><td rowspan="3">Name 02</td><td>001</td></tr>
    <tr><td>002</td></tr>
    <tr><td>003</td></tr>
  </tbody>
</table>

@mdo
Copy link
Member

mdo commented Jan 21, 2018

Hmm good point @ysds.

screen shot 2018-01-21 at 12 33 57 pm

@MartijnCuppens
Copy link
Member Author

Hmmm, that case is not fixable with utility-classes either. The only thing I could think of is adding a new class to the fields that are not aligned.

@MartijnCuppens
Copy link
Member Author

@mdo, is it an option to continue with this with a warning about the rowspan problem?

@ysds
Copy link
Member

ysds commented Feb 23, 2018

How about adding this as an optional class like .list-group-flush? I think default Bootstrap style should not force the case that causing broken space despite correct HTML markup.

@MartijnCuppens
Copy link
Member Author

@ysds, where would you add this class and what would this exactly do? Can you maybe provide a demo?

@MartijnCuppens
Copy link
Member Author

.not-first-cell and .not-last-cell classes can now be added to prevent unexpected behaviour with rowspans.

https://codepen.io/MartijnCuppens/pen/JMyOgv

@MartijnCuppens
Copy link
Member Author

@ysds & @mdo, what do you think about the latest changes?

@ysds
Copy link
Member

ysds commented Aug 8, 2018

I am worried that adding new features (or changing padding/margin) to existing class (.card> .table) will include the possibility of causing breaking changes. Also need to consider the .table-responsive. So I prefer something additional class: e.g. add class (.card-table or .table-card or etc.) to the .table in the .card

@MartijnCuppens
Copy link
Member Author

MartijnCuppens commented Aug 20, 2018

Good feedback, @ysds! I've now changed the selector to .card-table and added support for .table-responsive.

Also added an example of the responsive table in the demo: https://codepen.io/MartijnCuppens/pen/JMyOgv

@XhmikosR XhmikosR requested review from mdo and removed request for mdo September 18, 2018 12:37
@XhmikosR XhmikosR requested a review from a team as a code owner November 5, 2018 07:55
@royklutman
Copy link
Contributor

When not using a .card-header there seems to be a double top border, when the .card-table is the first child, to me that looks a little off: https://codepen.io/royklutman/pen/bQwdOW (first card). This could be fixed with this:

.card-table {
  &:first-child th {
    border-top: 0;
  }
}

Working example: https://codepen.io/royklutman/pen/jQMPda (first card).

@MartijnCuppens
Copy link
Member Author

@royklutman, good point, but the solution is not that simple, we'll also need to cover these cases:

  • If the th is added in the tbody, it should have a top-border (unless there is no thead and the th is the is in the first tr of the tbody)
  • The top border from tbody:first-child tr:first-child td should also be removed
  • Also support .table-responsive

So in the end we'll end up with this css:

.card > .card-table:first-child > thead:first-child tr:first-child th,
.card > .card-table:first-child > tbody:first-child tr:first-child th,
.card > .card-table:first-child > tbody:first-child tr:first-child td,
.card > .table-responsive:first-child .card-table > thead:first-child tr:first-child th,
.card > .table-responsive:first-child .card-table > tbody:first-child tr:first-child th,
.card > .table-responsive:first-child .card-table > tbody:first-child tr:first-child td {
  border-top: none;
}

I'm not sure if it's worth adding this much CSS for that case, especially when the top border can be removed with utility classes.

@mdo
Copy link
Member

mdo commented Jan 4, 2019

@MartijnCuppens Would you be okay with holding off on this until v5 maybe? It looks great overall, but I'm wondering if there's something we can do in the default component padding values to better support these tables without extra CSS.

@MartijnCuppens
Copy link
Member Author

Changing the default paddings would indeed be a better option, I'll remove this from v4.3.

@MartijnCuppens
Copy link
Member Author

Gonna close this, syncing the paddings in v5 would be a better option.

@florianlacreuse
Copy link
Contributor

@MartijnCuppens @mdo Will you reconsider this feature now that Bootstrap 5 is out?

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.

6 participants