New Streams Table Column Sizing Fix #20141
Conversation
octavia-squidington-iv added area/platform 
airbyte-webapp/src/components/SimpleTableComponents/SimpleTableComponents.tsx Outdated Show resolved Hide resolved
| word-break: break-word; | ||
| color: ${({ theme, light, lighter }) => (light ? theme.greyColor40 : lighter ? theme.greyColor60 : "inherit")}; | ||
| font-weight: ${({ light, lighter }) => (light || lighter ? "normal" : "inherit")}; | ||
| color: ${({ theme, light }) => (light ? theme.greyColor60 : "inherit")}; |
There was a problem hiding this comment.
light was unused so I renamed lighter to light and removed the light functionality.
| Did a timebox on repeating this on the old table. It's a touch more complex than the new table use-case and I don't feel it's worth more time. |
| import styles from "./CatalogTreeSubheader.module.scss"; | ||
| | ||
| const SubtitleCell = styled(Cell).attrs(() => ({ lighter: true }))` | ||
| const SubtitleCell = styled(Cell).attrs(() => ({ light: true }))` |
There was a problem hiding this comment.
This was not caught by the rename! Can't wait to get rid of styled components.
…-table-column-sizing
…-table-column-sizing
| children, | ||
| }) => { | ||
| return <div className={classNames(styles.tableCell, sizeMap[size])}>{children}</div>; | ||
| }; |
There was a problem hiding this comment.
New cell component for these tables
| margin-bottom: 29px; | ||
| max-height: 600px; | ||
| overflow-y: auto; | ||
| min-width: fit-content; |
There was a problem hiding this comment.
Absolutely core to this working.
| | ||
| .tableCell { | ||
| flex: 0 0 $medium; | ||
| min-width: $medium; |
There was a problem hiding this comment.
Need to pair this with the flex-basis for it to work.
edmundito left a comment
edmundito left a comment There was a problem hiding this comment.
Looks great! I tested with the new table and the old table as well as bulk edit for both.
Unfortunately, there's one more thing. While expanding the old table with the fields, I found this:
Looks like setting the CatalogTree to |
| Tested it and found an issue with the old and new table: |
This issue exists in prod so I made a ticket. |
| I tested again and it looks good despite the checkbox issue. One thing I noticed is that the table headers were always visible, but they're not anymore. Was this a conscious decision? |
This is likely due to needed to move the header into the same containing div as the body so the flex-width matched. So it was not explicitly done but is likely not an easy revert. |
…-table-column-sizing
airbyte-webapp/src/components/connection/CatalogTree/next/CatalogTreeTableRow.module.scss Show resolved Hide resolved
edmundito left a comment
edmundito left a comment There was a problem hiding this comment.
Looks good. Tested both tables locally.
Discussed with @krishnaglick about not having sticky headers for now and making any other detail improvements separately.
…-table-column-sizing
What
Resolves #17937
How
Explicitly sets the widths for the columns, and prevents growing and shrinking.
Cleans up Cell component properties