Skip to content
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

Merged
merged 6 commits into from
Nov 27, 2023

Conversation

Bluepuper
Copy link
Contributor

No description provided.

@gravity-ui-bot
Copy link
Contributor

Playwright Test Component is ready.

@gravity-ui-bot
Copy link
Contributor

Preview is ready.

@EgorKluch
Copy link

Please, add to storybook example of rows with multiline text.

@Bluepuper
Copy link
Contributor Author

Please, add to storybook example of rows with multiline text.

added

@Bluepuper
Copy link
Contributor Author

Ended up aligning the content to the top of the cell
Screenshot 2023-11-22 at 12 18 53

@@ -55,6 +55,7 @@
border-bottom: 1px solid var(--g-color-line-generic);

line-height: variables.$tableLineHeight;
vertical-align: top;
Copy link
Contributor

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

Copy link
Contributor Author

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
Copy link
Contributor

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

Copy link
Contributor Author

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;
Copy link
Contributor

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

Copy link
Contributor Author

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 {
Copy link
Contributor

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

@Bluepuper Bluepuper merged commit 8011430 into main Nov 27, 2023
4 checks passed
@Bluepuper Bluepuper deleted the fix/selection_table_checkbox_expand branch November 27, 2023 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants