-
Notifications
You must be signed in to change notification settings - Fork 13
feat: improve appearing controls styles #1436
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
Conversation
<Button size="s" href={href} className={b({visible}, className)} target="_blank"> | ||
<Icon data={ArrowUpRightFromSquare} /> | ||
<Button size={size} href={href} className={b({visible}, className)} target="_blank"> | ||
<Icon data={ArrowUpRightFromSquare} size={buttonSizeToIconSize[size]} /> |
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 suppose we can use Button Icons as here
<Button size="xs">
<Button.Icon>
<ArrowsRotateLeft />
</Button.Icon>
</Button>;
and icon size should be set automatically according to button size
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.
Discussed with uikit team, button doesn't affect it's icon size. We should explicitly set icon size we want.
src/containers/App/App.scss
Outdated
@@ -93,9 +93,14 @@ body, | |||
.ydb-paginated-table__row, | |||
.ydb-tree-view__item { | |||
&:hover { |
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.
Since you add padding only here, you should also add padding for focused state. Now if I select button with Tab, it won't have right padding
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 refactored styles. Wrapper is only needed on hover. If buttons are focused from keyboard, they should be shown separately.
{additionalControls && ( | ||
<span className={b('additional-controls')}>{additionalControls}</span> | ||
)} |
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.
DeveloperUIButton
is controlled by display: none
and display: inline-block
. It cannot be selected with Tab. Two ways possible:
-
All buttons in the UI should be keyboard accessible, so this
DeveloperUIButton
button should be updated too -
We don't need to make buttons in table keyboard accessible (for me in tables it doesn't make much sense anyway), so we can stick to previous solution, but update only
ClipboardButton
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.
Lets try the hardest way number 1. I hope I've found a solution.
b76bf77
to
898e973
Compare
…utton component in favor of uikit version
8bbe3ef
to
45fe48b
Compare
closes #1045
Stand
CI Results
Test Status: ✅ PASSED
📊 Full Report
Bundle Size: 🔽
Current: 78.94 MB | Main: 78.97 MB
Diff: 0.03 MB (-0.04%)
✅ Bundle size decreased.
ℹ️ CI Information