Table fixed layout
#2545
Merged
Table fixed layout
#2545
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
Size Change: +579 B (+0.04%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
shaneeza
commented
Nov 18, 2024
| ) => | ||
| cx(baseStyles, themeStyles[theme], bodyTypeScaleStyles[baseFontSize], { | ||
| [css` | ||
| //TODO: add to documentation |
There was a problem hiding this comment.
Will add in documentation PR
…fygreen-ui into shaneeza/table-fixed
…fygreen-ui into shaneeza/table-fixed
stephl3
reviewed
Nov 19, 2024
Comment on lines
702
to
707
| className={css` | ||
| ${index === 2 && | ||
| css` | ||
| width: auto; | ||
| `} | ||
| `} |
There was a problem hiding this comment.
can we add a comment here about why this is needed?
There was a problem hiding this comment.
In the Table/WithVirtualizedScrolling/DifferentHeights example, this is a clip toggling between the 1440px and 1024px breakpoints
https://github.com/user-attachments/assets/5ce95740-7a77-449f-a344-52054244cdb0
This width: auto; is causing the "Cluster Type" column width to shrink and because it's the most text-heavy data, it creates an appearance of a vertical gap in some rows
There was a problem hiding this comment.
I think that this is just an unfortunate side effect of using table layout fixed with width auto. To avoid this, consumers will need to set a static width.
There was a problem hiding this comment.
I see, can we update this story to make it more like what might get used in prod for internal testing purposes? As is, I worry if it will be difficult to know in the future if this is a bug in the package vs in the story. Maybe a different column can be used to show responsiveness and we can add a min-width, so it doesn't shrink to 0?
There was a problem hiding this comment.
@shaneeza tagging in case my reply got buried because I replied in thread rather than in a separate review
There was a problem hiding this comment.
Thanks for tagging me. I wanted to go off of evergreen and make the column with the heaviest data responsive. I could move columns around so the first column has the heaviest data. And I could also stack the number of rows so the the table has more space.
There was a problem hiding this comment.
Also, i forgot, It doesn't look like min-width works with a fixed layout
There was a problem hiding this comment.
gotcha, I still think it's strange that the column can shrink to 0 at smaller breakpoints, but good to go if that's expected / communicated that that can happen
There was a problem hiding this comment.
I'll document it in my documentation PR
| @@ -19,6 +19,8 @@ export const getBaseHeaderCellStyles = (size: number, isSelectable?: boolean) => | |||
| &:first-of-type { | |||
| ${getCellPadding({ depth: 0, isExpandable: false, isSelectable })} | |||
| } | |||
|
|
|||
| line-height: 16px; | |||
There was a problem hiding this comment.
curious why this is added? also seeing an explicit height set in headerCellContentStyles so wondering if only 1 is needed
There was a problem hiding this comment.
Rob asked me to add it - he felt the spacing could be tighter. I tried 1, but it's tighter than 16px. Also, we don't have any tokens that match this line-height.
Base automatically changed from
shaneeza/table-sticky-shadow
to
table-refactor-integration
November 20, 2024 15:11
stephl3
approved these changes
Nov 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
✍️ Proposed changes
🎟 Jira ticket: Name of ticket
✅ Checklist
For bug fixes, new features & breaking changes
yarn changesetand documented my changesFor new components
🧪 How to test changes
Click on the
Different Heightsstory underTable/With Virtualized Scrolling. Open the story outside of an iframe and start scrolling.