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

Table: active <tr> fix #37084

Merged
merged 12 commits into from Apr 26, 2023
Merged

Conversation

louismaximepiton
Copy link
Member

@louismaximepiton louismaximepiton commented Sep 5, 2022

Fixes #33913.
May close #35158.

Based on ffoodd's idea.

Live preview

Solutions

Commits can be split.

On preview is:
image

  • The third one is just a typo error inside the doc.

Checks

  • No regression on table page and surroundings. Couldn't spot any but haven't dig too much.
  • No regression on table variants. Couldn't spot any but haven't dig too much.

@mdo
Copy link
Member

mdo commented Sep 6, 2022

Love the approach, but renaming CSS variables is a breaking change I think. We could keep this for v6, or we could try to address that and ship in v5.3. Thoughts?

@louismaximepiton louismaximepiton marked this pull request as draft September 7, 2022 07:32
@louismaximepiton
Copy link
Member Author

louismaximepiton commented Sep 7, 2022

IMO, it should be in v5 since folks can't have an .active cell or row inside a striped table.

I put this PR in draft since it requires more work to make it smoother for users but I don't think that we are changing variables with this PR (apart the accent one), we are adding some of them since previous variables will still be usable + I've seen a regression on striped tables that would be declared with a --bs-table-bg, they aren't striped anymore. I'll try to check that asap!

EDIT: I'll try to make no breaking change with the actual variables.

scss/_tables.scss Outdated Show resolved Hide resolved
scss/_variables.scss Outdated Show resolved Hide resolved
@louismaximepiton
Copy link
Member Author

I think this version is way better since we don't rename the actual variables, we just introduce more of them. Furthermore, you can still give a special color by overriding the associated CSS var, it'll still continue to work correctly.

I can't see any regression atm, shall I set this PR as open for 5.3.0?

@louismaximepiton louismaximepiton marked this pull request as ready for review September 12, 2022 07:51
@mdo
Copy link
Member

mdo commented Oct 30, 2022

Fixed the syntax to match our variables everywhere else, but with the main merge we have a bundlewatch issue. Can you take a pass to update @louismaximepiton?

@XhmikosR
Copy link
Member

@louismaximepiton can you rebase please?

@mdo mdo merged commit 5400415 into twbs:main Apr 26, 2023
15 checks passed
@@ -731,7 +731,7 @@ $table-cell-padding-x-sm: .25rem !default;
$table-cell-vertical-align: top !default;

$table-color: var(--#{$prefix}body-color) !default;
$table-bg: transparent !default;
$table-bg: var(--#{$prefix}body-bg) !default;
Copy link
Sponsor Contributor

@tkrotoff tkrotoff Jun 10, 2023

Choose a reason for hiding this comment

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

This is a breaking change.

Table background has been transparent since 2012

Also table-primary & table-secondary are available but not table-tertiary, why?

@louismaximepiton

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey, thanks a lot for your feedback!

I think I changed the default background from transparent to var(--bs-body-bg) because the nesting tables was broken if I remember well. I'm not sure how to handle this. If you have any other idea please don't hesitate to add a PR.

For .table-tertiary, it doesn't exists yet since our components (buttons, list-group, ...) don't have such a variant. I think if it's done for one, it might be done on all sides at the same time.

Copy link
Member

Choose a reason for hiding this comment

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

Confirming the part regarding the table variants. It's mentioned in our Tables > Variants documentation:

Heads up! Because of the more complicated CSS used to generate our table variants, they most likely won’t see color mode adaptive styling until v6.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Needs review
Status: Done
6 participants