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

WIP: Simplifies and improves table backgrounds #24660

Closed
wants to merge 2 commits into from

Conversation

andresgalante
Copy link
Collaborator

This PR is needs more clean up and better organization, but bare with me.

The way we use selectors for background on tables doesn't allow to have a different colors within a row, for example:

    <tr class="table-danger">
        <th scope="row">9</th>
        <td>Column content</td>
        <td class="table-warning">Column content</td>
        <td>Column content</td>
      </tr>

It also doesn't allow to mark something as active like this:

      <tr class="table-danger">
        <th scope="row">9</th>
        <td>Column content</td>
        <td class="table-active">Column content</td>
        <td>Column content</td>
      </tr>

or to mark something as active within an active row like this:

      <tr class="table-active">
        <th scope="row">9</th>
        <td>Column content</td>
        <td class="table-active">Column content</td>
        <td>Column content</td>
      </tr>

... and it produces issues like the one described on #24529

By simplifying the selectors it resolves it and I don't see conflicts with nested tables or .table-striped. I tried to digg up the history of this selector but I couldn't find the root.

@mdo @XhmikosR Can you please point me out what I am missing so I can continue with this PR?

Thanks a lot!

This is a WIP but when it's done it will close #24529

@XhmikosR
Copy link
Member

XhmikosR commented Nov 3, 2017

I'm definitely in favor of this, but I always thought there was some specificity reason that led to the current solution. Only @mdo can confirm :)

@mdo
Copy link
Member

mdo commented Nov 6, 2017

I think the move from Less to Sass is what broke these selectors. I can't recall any other background on why it's coded the way it is, so I'm definitely open to changing it.

@mdo
Copy link
Member

mdo commented Nov 6, 2017

Ah, just read the deleted comment again—these don't work with .table-striped now.

screen shot 2017-11-06 at 2 02 47 pm

@andresgalante
Copy link
Collaborator Author

@mdo right! I'll work on it

@andresgalante andresgalante force-pushed the v4-dev-andres-table-active-state branch from 53c8664 to 05919cc Compare November 7, 2017 15:12
@andresgalante
Copy link
Collaborator Author

@mdo This is a horrible solution, but it fixes the problem.

At the moment a stripe table will overide active states:
screen shot 2017-11-07 at 12 11 15 pm

And it has all the problems described above.

This horrible selector is needed to get our way around tables when table-stripe is on.

@XhmikosR @mdo can you please take another spin on this one and see what I am missing, or if we can improve the selector

@@ -70,6 +70,13 @@
.table-striped {
tbody tr:nth-of-type(odd) {
background-color: $table-accent-bg;
/* stylelint-disable */
Copy link
Member

Choose a reason for hiding this comment

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

@andresgalante: please be more explicit with what you disable here.

@@ -139,6 +144,13 @@
&.table-striped {
tbody tr:nth-of-type(odd) {
background-color: $table-dark-accent-bg;
/* stylelint-disable */
Copy link
Member

Choose a reason for hiding this comment

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

...and here.

@mdo
Copy link
Member

mdo commented Jan 16, 2018

Quick update from me—I've been trying to look into this here and there to see what else can be done with our selectors. Still trying, but not a top priority for me or block to v4 stable right now.

@mdo
Copy link
Member

mdo commented Feb 14, 2019

Closing as stale and as something to revisit in v5.

@mdo mdo closed this Feb 14, 2019
@mdo mdo deleted the v4-dev-andres-table-active-state branch February 14, 2019 16:56
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

3 participants