-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Table
column vertical alignment
#2547
Table
column vertical alignment
#2547
Conversation
…fygreen-ui into shaneeza/table-column-vertical-alignment
|
Size Change: +98 B (+0.01%) Total Size: 1.36 MB
ℹ️ View Unchanged
|
…green-ui into shaneeza/table-column-vertical-alignment
className={cx(baseCellStyles, className)} | ||
className={cx(getBaseCellStyles(verticalAlignment), className)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: lately, I've been consuming @leafygreen-ui/emotion
only in the *.styles.ts
file and passing all variables including className
into the style function. In this case, you end up with something like getCellStyles(className, verticalAlignment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this pattern. However, I'm going to keep it as is. If I have extra time, I'll go back and update all table styles to match this pattern.
@@ -242,7 +271,6 @@ DisabledClickableRows.args = { | |||
export const DisabledSelectableRows: StoryFn< | |||
RowProps<Person> & DarkModeProps | |||
> = ({ darkMode, ...args }: DarkModeProps & RowProps<Person>) => { | |||
const tableContainerRef = React.useRef<HTMLDivElement>(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not necessary for a regular table
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
✍️ Proposed changes
🎟 Jira ticket: Name of ticket
In this PR, I added a new prop to
Table
calledverticalAlignment
. This prop controls the vertical alignment of rows that span multiple lines. The available options areTop
(default) andMiddle
.✅ Checklist
For bug fixes, new features & breaking changes
yarn changeset
and documented my changesFor new components
🧪 How to test changes in Storybook
Table
>No Truncation
: toggleverticalAlignment
Table
>With Virtualized Scrolling
>No Truncation
: toggleverticalAlignment