-
Notifications
You must be signed in to change notification settings - Fork 12
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
feat(data-grid): support dynamic row height (VIV-1602) #1641
Conversation
0a869bd
to
d631245
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1641 +/- ##
===========================================
Coverage 100.00% 100.00%
===========================================
Files 123 320 +197
Lines 1562 5375 +3813
Branches 108 652 +544
===========================================
+ Hits 1562 5375 +3813
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -31,11 +33,15 @@ | |||
display: flex; | |||
box-sizing: border-box; | |||
align-items: center; | |||
padding: 8px 12px; | |||
padding: 14px 12px; |
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.
padding: 14px 12px; | |
padding: 12px; |
14px is not on the 8px scale...
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.
The value is taken from the current design in Figma. I've asked Ayala to comment on it.
However, I don't think it really matters, as changing the margin would be a breaking change and is unrelated to this ticket.
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 don't think it will be a breaking change as cell has min-height of 48px, so it can be either way.
And only if its bigger then 48px then the padding make a difference.
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 don't think it will be a breaking change as cell has min-height of 48px, so it can be either way.
And only if its bigger then 48px then the padding make a difference.
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.
Yes, but I think the padding should always be the same
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.
@AyalaBu ?
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.
the padding should be left 14px
Co-authored-by: Rachel Bratt Tannenbaum <rachelbt@users.noreply.github.com>
Co-authored-by: Rachel Bratt Tannenbaum <rachelbt@users.noreply.github.com>
No description provided.