Group table body styles #3681
Group table body styles #3681
Conversation
| @@ -22,7 +22,7 @@ export const AddStage: React.FC<AddStageProps> = ({ hasValidDvcYaml }) => ( | |||
| disabled={!hasValidDvcYaml} | |||
| /> | |||
| {!hasValidDvcYaml && ( | |||
| <p className={styles.error}> | |||
There was a problem hiding this comment.
Similiar to the previous pr, renamed some classes to better explain what they're doing :)
| } | ||
|
|
||
| .experimentCellContentsContainer { | ||
| @extend .cellContents; |
There was a problem hiding this comment.
Personally, I don't think it's the best practice to extend regular classes because someone could update the class not realizing they're effecting other classes. Created another one of our "Extendable Silent Rules" for this.
| @@ -804,13 +748,18 @@ td { | |||
| .experimentCellSecondaryName { | |||
| color: $meta-cell-color; | |||
| font-size: 0.75em; | |||
| display: flex; | |||
| align-items: center; | |||
|
|
|||
There was a problem hiding this comment.
Noticed that the text-overflow wasn't working. Fixed now:
|
|
||
| &:not(.experimentCell)::before { | ||
| @extend %cellBorderLeft; | ||
| } | ||
|
|
||
| .rowSelected & { |
There was a problem hiding this comment.
I think it's a little better but the way we update backgrounds/borders for cells is still kind of complex. I think there are some better ways to go about this but I've already spent quite a bit of time on updating table styles, so will look at it at a later date!
|
|
||
| .innerCell { | ||
| padding: 5px 10px; |
There was a problem hiding this comment.
I noticed that we have the workspace cells slightly misaligned with the other rows. I assumed it wasn't intentional and deleted the line.
| @@ -80,17 +80,9 @@ $badge-size: 0.85rem; | |||
| background-color: $header-border-color; | |||
There was a problem hiding this comment.
Looked at storybook, and apparently, storybook applies selectors from table/styles.module.scss. to the plots stories. With the code outside of the .experiments wrapper, the plots stories are now breaking.
Are we already aware of this issue in storybook? I'm not sure why it's happening yet, but if it's hard to fix, we could:
- move everything back into an
experimentswrapper. While annoying to have another wrapper around everything, it would be the quickest way to fix the problem. - Don't select elements at all, just rely on classes. We'd need to add some extra classes like
.tdand.thand update the file accordingly.
There was a problem hiding this comment.
Discuss this on our call. Need to understand why these were being applied (seems like a bug in the way we've setup the stories). What is shown in the extension should be what is shown in Storybook.
There was a problem hiding this comment.
I agree with Matt, this looks like a bug in how we setup the stories. Couldn't find where we directly import the table styles, but we directly import the plots one. We should probably clean that up and find out where these styles are coming from. That said, I think relying on classes instead of elements is good practices. I'm the one to blame here for that. During the react-table upgrade I changed all the divs to table elements and removed the .td, .th classes because they weren't on the right elements. I should have moved them instead.
3/4
main<= #3667 <= #3674 <= this <= #3707Part of #3597