Skip to content

Commit

Permalink
Fix: #290 table-striped and table-hover should work with table-list
Browse files Browse the repository at this point in the history
  • Loading branch information
@pat270
pat270 committed Nov 29, 2016
1 parent aa167d2 commit dc862bd
Showing 1 changed file with 14 additions and 0 deletions.
14 changes: 14 additions & 0 deletions src/scss/lexicon-base/_tables.scss
View file
Expand Up @@ -108,6 +108,8 @@ th {
> tfoot > tr > td,
> tfoot > tr > th {
@if (variable-exists(atlas-theme)) {
// Chrome rendering issue with responsive-tables and td position relative

background-color: $table-list-body-row-bg;
}

Expand Down Expand Up @@ -151,6 +153,18 @@ th {
}
}

@if (variable-exists(atlas-theme)) {

This comment has been minimized.

Sign in to view

Copy link
@jbalsas

jbalsas Dec 1, 2016

Contributor

Hey guys, we've seen some this @if (variable-exists(atlas-theme)) in several places now... what's the reasoning for it? From an outside point of view, seems like we should never do this and couple lexicon base with a specific implementation such as atlas...

This comment has been minimized.

Sign in to view

Copy link
@pat270

pat270 Dec 1, 2016

Author Member

Hey Chema,

This was a concession for reducing the number of css selectors for IE9 compatibility. Atlas styles deviate so much from Bootstrap and when combined with Portal's theme inheritance, it requires us to add a lot of extra selectors. Doing this helps alleviate the number of selectors.

This comment has been minimized.

Sign in to view

Copy link
@jbalsas

jbalsas Dec 21, 2016 •
edited

Contributor

Hey Patrick, thanks a lot for the explanation.

I think this looks like a valid compromise for now, but we need to start rethinking this approach, since we're going to be creating more skins (and can't keep adding ifs and variables here).

We might also want to start rethinking about future browser support... what should Lexicon2 support? how will Bootstrap 4 affect that? If we decide to drop some, this might become less important in the future...

This comment has been minimized.

Sign in to view

Copy link
@pat270

pat270 Dec 21, 2016

Author Member

We wouldn't have to add any @if's for skins since they will be based on Lexicon Base or Atlas. As for browser support, it depends on what is decided for portal. Bootstrap 4's minimum desktop browser is IE9.

This comment has been minimized.

Sign in to view

Copy link
@jbalsas

jbalsas Dec 21, 2016

Contributor

If Bootstrap 4 supports IE9, then it makes sense for us to do it as well. I'm not so sure about the skins yet... to me somethings smells fishy on the build level. We might be able to just build it in a way that only the necessary stuff is added, but of course, I can't really tell. Maybe @Robert-Frampton could weigh in on this and start thinking about it as well.

This is okay for now, but just food for thought for the future... we can take a closer look after christmas :)

This comment has been minimized.

Sign in to view

Copy link
@yuchi

yuchi Dec 21, 2016

@jbalsas Bootstrap 4 drops support for IE9. I though this could be interesting.

This comment has been minimized.

Sign in to view

Copy link
@jbalsas

jbalsas Dec 21, 2016 •
edited

Contributor

hahaha.. yeah, we were commenting on it earlier on this afternoon... that actually might make things harder for us as far as Liferay DXP is concerned, since we're bound to maintain IE9 support, so we at least won't be able to simply swap the implementation and will need to offer something like an opt-in or compatibility layer for it...

but yeah, definitely interesting times 😉

This comment has been minimized.

Sign in to view

Copy link
@yuchi

yuchi Dec 21, 2016

We saw a huge drop in IE9 users. Take your time before choosing to keep suppport for it.

This comment has been minimized.

Sign in to view

Copy link
@jbalsas

jbalsas Dec 21, 2016 •
edited

Contributor

nah, we'll be dropping it for sure. I just meant that we'll have to think about how can we roll out an update that will change our browser support matrix... I'll actually start asking around tomorrow to see which options we may have...
// Chrome rendering issue with responsive-tables and td position relative

.table-striped > tbody > tr:nth-of-type(odd) > td {
background-color: $table-bg-accent;
}

.table-hover > tbody > tr:hover > td {
background-color: $table-bg-hover;
}
}

// Table Helpers

.table-autofit {
Expand Down

0 comments on commit dc862bd

Please sign in to comment.