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

feat(Table): expand checkbox click area #654

Merged
merged 10 commits into from
Oct 19, 2023

Conversation

Bluepuper
Copy link
Contributor

Issue: #648

@Bluepuper Bluepuper requested a review from krozhkov as a code owner May 2, 2023 10:12
@gravity-ui-bot
Copy link
Contributor

Preview is ready.

@teleginzhenya
Copy link
Contributor

@krozhkov can you take a look on this pr?

@teleginzhenya
Copy link
Contributor

@amje maybe you? lgtm :)

src/components/Table/Table.scss Outdated Show resolved Hide resolved
src/components/Table/Table.scss Outdated Show resolved Hide resolved
@Bluepuper
Copy link
Contributor Author

@amje Can you check please?

@amje
Copy link
Contributor

amje commented Aug 2, 2023

@Bluepuper
There are some issues when Table is overflowing

@Bluepuper Bluepuper force-pushed the feat/selection_table_checkbox_expand branch from 832a178 to 8e360f2 Compare September 28, 2023 08:32
@Bluepuper
Copy link
Contributor Author

@Bluepuper There are some issues when Table is overflowing

Add min-width of checkbox to cell, so it will not be shrinking

src/components/Table/Table.scss Outdated Show resolved Hide resolved

#{variables.$block} {
&__selection-checkbox {
display: inline-block;
@extend #{variables.$block}__cell;
@extend #{variables.$block}__cell_edge-padding;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about styles when edgePadding: false?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove duplicating styles

@@ -152,6 +152,7 @@ export class Table<I extends TableDataItem = Record<string, string>> extends Rea
content = id;
}

if (id === '_selection') return content;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did this to remove double wrapping of checkbox so padding: inherit will work

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But now selection hoc "data" leaked to the base Table. Let's find a workaround here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this wrapper really breaks the layout?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is one with additional wrapper
Screenshot 2023-10-10 at 21 25 18

this is one without
Screenshot 2023-10-10 at 21 26 24

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I add padding: inherit to the th-content class, I can fix the existence of the extra wrapper, but then all the other cells are broken
Screenshot 2023-10-10 at 21 27 38

Copy link
Contributor Author

@Bluepuper Bluepuper Oct 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i can add different wrapper for cell with id === '_selection', something like this:
if (id === '_selection') return <span className={b('th-content_selection')}>content</span>;

before if was just like this return <span className={b('th-content')}>{content}</span>

Copy link
Contributor

@amje amje Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Bluepuper I think we can remove this wrapper (th-content) completely, and add these styles instead:

    &__head &__cell {
        @include mixins.text-accent;
        &::first-letter {
            text-transform: uppercase;
        }
    }

Copy link
Contributor Author

@Bluepuper Bluepuper Oct 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove this wrapper (th-content) completely, and add these styles instead:

    &__head &__cell {
        @include mixins.text-accent;
        &::first-letter {
            text-transform: uppercase;
        }
    }

like this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Bluepuper
Copy link
Contributor Author

Can you check it please ?) @amje

@amje amje changed the title feat(SelectionTable): expand checkbox click area feat(Table): expand checkbox click area Oct 18, 2023
@Bluepuper Bluepuper force-pushed the feat/selection_table_checkbox_expand branch from af2ecc1 to 07e0f1c Compare October 19, 2023 10:37
@Bluepuper Bluepuper merged commit 0534cf8 into main Oct 19, 2023
3 checks passed
@Bluepuper Bluepuper deleted the feat/selection_table_checkbox_expand branch October 19, 2023 10:40
@EgorKluch
Copy link
Contributor

Is it ok that checkbox isn't in center of a cell anymore?
Screen Shot 2023-11-21 at 11 04 27

@amje
Copy link
Contributor

amje commented Nov 21, 2023

Is it ok that checkbox isn't in center of a cell anymore? Screen Shot 2023-11-21 at 11 04 27

@EgorKluch I assume, yes. Now it's an equal padding between all cells

@EgorKluch
Copy link
Contributor

EgorKluch commented Nov 21, 2023

@EgorKluch I assume, yes. Now it's an equal padding between all cells

But then needs to move up text too.

I'm waiting for the final decision.

@amje
Copy link
Contributor

amje commented Nov 21, 2023

@EgorKluch Oh, I thought you're talking about horizontal alignment. Checkbox should be centered as well when verticalAlign: center is set

@Bluepuper
Copy link
Contributor Author

Bluepuper commented Nov 21, 2023

@amje Aligned the content to the top when there is two lines or more in cell in this PR, based on the design and on the ticket

@EgorKluch
Copy link
Contributor

Also change column height in some cases. I send links of example in telegram.
image

@Bluepuper
Copy link
Contributor Author

Also change column height in some cases. I send links of example in telegram. image

It turns out that the row height was wrong before this pr based on the design. This pr also fixes that, now it is correct: 40px height + 1px bottom border. Before it was 40.5 px inner height + 1px border bottom

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.

5 participants