-
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
Add focus styles to FileInput #6950
Conversation
Generate changelog in
|
Add focus state styles to file inputBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
8bf4fa5
to
c50a4a4
Compare
Add focus state styles to file inputBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
&:focus + .#{$ns}-file-upload-input { | ||
box-shadow: input-transition-shadow($input-shadow-color-focus, true), | ||
$input-box-shadow-focus; | ||
|
||
.#{$ns}-dark & { | ||
box-shadow: dark-input-transition-shadow($dark-input-shadow-color-focus, true), | ||
$pt-dark-input-box-shadow; | ||
} | ||
} |
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.
is this a paired down version of the pt-input-intent
mixin? maybe we could comment that if so to at least loosely link these
also wondering if we should be using the same 2px
found in that mixin for consistency? is there a reason these should be different?
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.
The box shadows here were pulled from the base pt-input
mixin:
blueprint/packages/core/src/components/forms/_common.scss
Lines 109 to 112 in dc1b162
&:focus, | |
&.#{$ns}-active { | |
box-shadow: input-transition-shadow($input-shadow-color-focus, true), $input-box-shadow-focus; | |
} |
blueprint/packages/core/src/components/forms/_common.scss
Lines 59 to 73 in dc1b162
@function input-transition-shadow( | |
$color: $input-shadow-color-focus, | |
$focus: false, | |
$outer-shadow-focus-width: 1px, | |
) { | |
@if $focus { | |
@return | |
inset border-shadow(0.752, $color, 1px), | |
border-shadow(0.752, $color, $outer-shadow-focus-width); | |
} @else { | |
@return | |
border-shadow(0, $color, 0), | |
border-shadow(0, $color, 0); | |
} | |
} |
blueprint/packages/core/src/common/_mixins.scss
Lines 100 to 102 in c50a4a4
@function border-shadow($alpha, $color: $black, $size: 1px) { | |
@return 0 0 0 $size rgba($color, $alpha); | |
} |
blueprint/packages/core/src/components/forms/_common.scss
Lines 30 to 32 in dc1b162
// avoids edge blurriness for light theme focused default input | |
// second box-shadow of $pt-input-box-shadow | |
$input-box-shadow-focus: inset 0 1px 1px rgba($black, $pt-drop-shadow-opacity) !default; |
and the base
pt-dark-input
mixin:blueprint/packages/core/src/components/forms/_common.scss
Lines 200 to 202 in dc1b162
&:focus { | |
box-shadow: dark-input-transition-shadow($dark-input-shadow-color-focus, true); | |
} |
blueprint/packages/core/src/components/forms/_common.scss
Lines 75 to 89 in dc1b162
@function dark-input-transition-shadow( | |
$color: $dark-input-shadow-color-focus, | |
$focus: false, | |
$outer-shadow-focus-width: 1px, | |
) { | |
@if $focus { | |
@return | |
inset border-shadow(0.752, $color, 1px), | |
border-shadow(0.752, $color, $outer-shadow-focus-width); | |
} @else { | |
@return | |
border-shadow(0, $color, 0), | |
border-shadow(0, $color, 0); | |
} | |
} |
The default focus box shadow on input groups without an intent appears to be 2px total (1px inset + 1px).
For input groups with an intent, it appears to be 3px (1px inset + 2px)
I chose to go with the non-intent style box shadow for file input since it currently does not have the ability to have our intent styles applied. I do think it's a bit odd that the default, non-intent focus outline doesn't match the width of the focused outlines. Perhaps that's something to address in a FLUP? For now, I can comment to reference where these are coming from.
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 do think it's a bit odd that the default, non-intent focus outline doesn't match the width of the focused outlines.
ahh nice catch - I just recently updated these focus styles. looking back at this comment: #6854 (comment) I'm seeing originally I used a lighter color for the 2px which then became the same color (and a that point I probably just missed keeping it consistent at 2px)
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.
oh - maybe I did that because it already starts out with the 1px colored border when not focused
agreed without intent it makes sense for this to just be the 2px focus state!
Add commentBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Fixes #6859
Checklist
Changes proposed in this pull request:
Add visual focus styles to FileInput to match other input components.
Reviewers should focus on:
Screenshot
Focus (light mode)
Focus (dark mode)