DOP-5342: ListTable consolidates cell content inside of divs #1346
Conversation
✅ Deploy Preview for docs-frontend-dotcomstg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for docs-frontend-stg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| margin: 0 0 12px; | ||
| & > *, | ||
| & > div p { | ||
| margin: 0 0 12px !important; |
There was a problem hiding this comment.
@rayangler is there any way we can use another approach other than!important? if not, then it is what it is. !important is normally considered to be bad practice, and can make maintaining difficult down the road.
There was a problem hiding this comment.
Yeah, I'm not a fan of !important either. Honestly, I don't really understand it but when I was trying to work on it on local development, it was working fine without !important, but when statically built, the order of CSS was different, causing the styling of these components to be different as well. I couldn't think of or find a more straightforward way to handle it.
I can timebox 30-60 minutes at some point to look into this more to see if there are other options that don't require writing some roundabout CSS/JS logic. Would that work?
There was a problem hiding this comment.
@rayangler that would be perfect, but please don't spend anything more than what you mentioned above. I don't think it is worth eating up too much of your time.
There was a problem hiding this comment.
I spent some time looking into it but couldn't find a full fix for it that kept changes minimal. I've tried upgrading a couple of dependencies @leafygreen-ui/emotion and gatsby-plugin-emotion, and modifying the selectors to be a little bit more specific (this worked for some elements but not for others, and it might require an exhaustive list of HTML since tables can have any elements in them).
Some things I didn't try that might be interesting in the future would be:
- Pass some
table-cell-contentclassname down to all cell child components and then use those classnames as the selectors? (Unsure how well this would work with deeply nested components; we might have to make sure every component is passing that exact classname every time) - We have a context that can keep track if a component is inside of a table or not. Maybe we can leverage that and write some conditional css at the component level to handle the styling accordingly (this may be annoying to have to add to every possible nested component though...)
| @@ -308,7 +309,7 @@ const ListTable = ({ nodeData: { children, options }, ...rest }) => { | |||
| {headerGroup.headers.map((header) => { | |||
| return ( | |||
| <HeaderCell className={cx(baseCellStyle, headerCellStyle)} key={header.id} header={header}> | |||
| {flexRender(header.column.columnDef.header, header.getContext())} | |||
| <div>{flexRender(header.column.columnDef.header, header.getContext())}</div> | |||
There was a problem hiding this comment.
My only request: Maybe we leave a comment here and below to explain why the div is there so no one removes it one day.
There was a problem hiding this comment.
Good callout. This sounds like a div I'd accidentally remove
Stories/Links:
DOP-5342
Related Osiris PR
Current Behavior:
Test tables staging (main branch)
Staging Links:
Test tables staging page - should ideally look the same as the one on the main branch.
Notes:
README updates