Avoid CSS element selection in table styles #3707
Conversation
| @@ -197,10 +197,6 @@ table.withExpColumnShadow tr > *:first-child { | |||
| @extend %cellContentsBase; | |||
| } | |||
|
|
|||
| ul { | |||
There was a problem hiding this comment.
I don't think we are actually using this anywhere.
| background-color: $row-border-selected-color; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| .nestedRow { | ||
| .experimentCell .innerCell { |
There was a problem hiding this comment.
Moved these lines to fix a specificity issue but I also think it looks more readable :)
| @@ -113,18 +113,18 @@ $badge-size: 0.85rem; | |||
| flex-flow: column nowrap; | |||
| } | |||
|
|
|||
| table { | |||
| .table { | |||
There was a problem hiding this comment.
[Q] Do we want to prefix these with "experiments" to make it more obvious what they are for?
There was a problem hiding this comment.
I think the idea for these classes is that they apply to all table elements, just like they would if I was using element selectors like we were originally. But no huge preference, I can update them with a prefix!
There was a problem hiding this comment.
A little late to the discussion (we can keep the prefixes), but it's forgetting we are inside of a CSS module. The fact that it gives local names to classes and let us use small meaningful names without prefixing them is the main purpose of using CSS Modules. This cleanup is to move closer to best practices using CSS Modules.
There was a problem hiding this comment.
Agreed that we can simplify names when inside CSS modules but shouldn't that apply to cases when SCSS files are more broken up? Like one file is used for one or a group of similar components? With our current situation, one scss file being used for all experiments components, lengthy names can help to make more sense of things.
Though (I think you mentioned this already in one of these prs if I'm remembering correctly), if we go on to break up this file into more files, simplified names would easily make sense without any risk of confusion.
Again though, no huge preference, just my thoughts!
There was a problem hiding this comment.
This file is indeed huge, but we are still inside experiments. It's normal to distinguish between something like .headerRow and bodyRow as they would both be found inside of the file, but there is one table and this particular module is inside Experiments.
Like I said, no need to rechange them, but in the future, let's keep short names. If we end up having to use prefixes or suffixes for class names, it's a great sign that our components and style modules are modular enough. (It really is the case here, but we cannot solve it that easily).
4/4
main<= #3667 <= #3674 <= #3681 <= thisDoing this fixes issues we have with storybook making component styles global (see slack thread) and is a good practice in general (avoids accidental element selection and keeps specificity more equal across selectors).
Part of #3597