-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Refactor buttons borders #537
Conversation
Interesting, you're proposing going back to box shadows? What are the tradeoffs / remind me why we moved away from box shadows in the first place? |
Hehe, glad you're asking. Yes, we're basically going back to
Shadows solve all of the above problems, + more. Why did I even use borders in the first place? It was because we wanted the default light theme button to have their border outside of the background shape, which we can't achieve with a |
The shadow stuff from this PR is actually not that interesting because it's super straightforward and easy to understand. The meaty stuff is the z-index ordering in button groups and control groups |
@llorca Thanks for explaining. I think it would be really great to include that explanation of tradeoffs in code comments. |
@adidahiya I thought about it but didn't so appropriate for code comments. I can do it, however it will be quite a large block of comments to explain all of that |
@llorca yeah, a large block comment is totally reasonable, and IMO preferable when there are important design decisions that are hard to glean from just reading code |
Cool, I'll add a block comment describing the tradeoffs between borders and buttons 👍 |
Tried to keep the comment as short as possible. How's that? |
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.
🔥 this is so hot!
@@ -13,45 +13,67 @@ $button-border-width: 1px !default; | |||
$button-padding: 0 $pt-grid-size !default; | |||
$button-padding-large: 0 ($pt-grid-size * 1.5) !default; | |||
$button-icon-spacing: ($pt-button-height - $pt-icon-size-standard) / 2 !default; | |||
$button-icon-spacing-large: ($pt-button-height-large - $pt-icon-size-large) / 2 !default; | |||
$button-icon-spacing-large: ($pt-button-height-large - $pt-icon-size-standard) / 2 !default; |
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.
dbl check that this is correct
} | ||
|
||
&:active, | ||
&.pt-active { | ||
z-index: 4; | ||
z-index: 3; |
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.
brief code comments outlining the z-index layers here might be helpful
&:not(.pt-minimal) { | ||
> .pt-popover-target:not(:last-child) .pt-button, | ||
> .pt-button:not(:last-child) { | ||
margin-right: $button-border-width; |
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.
explain this huge selector
$button-intent-box-shadow-active: | ||
inset 0 0 0 $button-border-width rgba($black, 0.4), | ||
inset 0 1px 2px rgba($black, 0.2) !default; | ||
$button-box-shadow-overlay: |
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.
explain when to use -overlay vs regular shadows? something about shadows blending with elements beneath
Fixes #233, countless of unreported button groups and control groups visual issues
Changes proposed in this pull request:
$button-box-shadow-overlay
Reviewers should focus on:
disabled
buttons/inputs,readonly
inputs, and all kinds of weird combinations that you can think of.Screenshot
Example from #233.
Before:
After: