-
Notifications
You must be signed in to change notification settings - Fork 100
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
fix: align checkbox when there is more than 1 line of text #1149
Conversation
Playwright Test Component is ready. |
Preview is ready. |
Please, add to storybook example of rows with multiline text. |
|
src/components/Table/Table.scss
Outdated
@@ -55,6 +55,7 @@ | |||
border-bottom: 1px solid var(--g-color-line-generic); | |||
|
|||
line-height: variables.$tableLineHeight; | |||
vertical-align: top; |
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.
That is a breaking change. We use default alignment for column (which is center). This behaviour can be changed via verticalAlign
prop
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.
return ( | ||
<div> | ||
{item.name} <br /> | ||
Lorem |
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.
Better to split name by space instead of adding word
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.
@@ -7,13 +7,19 @@ | |||
} | |||
|
|||
&__selection-checkbox { | |||
display: flex; | |||
align-items: center; |
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.
Let's add an extra container for Checkbox and not put layout properties to the Checkbox itself, we can move positioning properties there as well
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.
But we have to expand the checkbox label tag over the entire cell to have an ability clicking in any cell place to toggle selection. So it seems like if i will wrap that Checkbox component and center it using this wrapper - this label tag(that we need to being extended) will not take all cell place because of "padding" that centers that checkbox component
padding: inherit; | ||
border-bottom: none; | ||
position: absolute; | ||
top: 0; | ||
bottom: 0; | ||
left: 0; | ||
right: 0; | ||
|
||
&_verticalAlign_top { |
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.
We prefer kebab-case in CSS
No description provided.