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 incorrect focus style size on InputControl #54394

Merged
merged 2 commits into from
Sep 13, 2023
Merged

Conversation

richtabor
Copy link
Member

@richtabor richtabor commented Sep 12, 2023

What?

Fixes incorrect focus style size on InputControl, using the same idea as on #33842.

Why?

Consistency.

Testing Instructions

  1. Open a page.
  2. Add a paragraph block.
  3. Add typography options.
  4. See all InputControls using the proper focus style.

Screenshots or screencast

Before After
CleanShot 2023-09-12 at 14 55 38 CleanShot 2023-09-12 at 14 48 00

@richtabor richtabor added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Sep 12, 2023
@richtabor richtabor self-assigned this Sep 12, 2023
@richtabor richtabor requested review from mirka, ciampo and a team September 12, 2023 19:00
@richtabor richtabor marked this pull request as ready for review September 12, 2023 19:01
Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

Just noting that this PR changes the border color from COLORS.ui.borderFocus (which is an alias from the 10% darker theme color) to COLORS.theme.accent (which is the standard theme color)

A few questions:

  • Is changing the focused border color from the darker to the normal shade of the accent color on purpose?
  • should we also update the value of the COLORS.ui.borderFocus variable?
  • for better consistency, should we also review all other instances where COLORS.ui.borderFocus is used in the package, and update those styles if needed?

(cc @jasmussen )

Thank you!

packages/components/CHANGELOG.md Outdated Show resolved Hide resolved
@jasmussen
Copy link
Contributor

Good questions, I can answer some:

Is changing the focused border color from the darker to the normal shade of the accent color on purpose?

Focus styles across the UI should be pure blueberry (i.e. accent color) and always 1.5px thick. I didn't actually notice that the focus style here was darker, but if it was this corrects an error there.

should we also update the value of the COLORS.ui.borderFocus variable?

Probably, yes, in fact there are currently only one or two exceptions to using the pure accent color in the UI, and at the moment all of those are related to lightening the accent color to work on the dark material. This will likely expand a bit in the future as that UI componentry matures a bit, but as a rule of thumb it's almost always the pure colors.

for better consistency, should we also review all other instances where COLORS.ui.borderFocus is used in the package, and update those styles if needed?

That would be great 🚀

Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

Such a good change, thank you, ship this as fast as you can!

Before:
Screenshot 2023-09-13 at 11 04 20

Screenshot 2023-09-13 at 11 04 41

After:

Screenshot 2023-09-13 at 11 06 06 Screenshot 2023-09-13 at 11 06 13 Screenshot 2023-09-13 at 11 06 17

@ciampo
Copy link
Contributor

ciampo commented Sep 13, 2023

always 1.5px thick

Thank you for the explanation.

It looks like this PR achieves the desired 1.5px thickness by combining a 1px border with a 0.5px box-shadow — is that the expected "technique" for focus styles that should be applied to all components?

should we also update the value of the COLORS.ui.borderFocus variable?

Probably, yes, in fact there are currently only one or two exceptions to using the pure accent color in the UI, and at the moment all of those are related to lightening the accent color to work on the dark material. This will likely expand a bit in the future as that UI componentry matures a bit, but as a rule of thumb it's almost always the pure colors.

for better consistency, should we also review all other instances where COLORS.ui.borderFocus is used in the package, and update those styles if needed?

That would be great 🚀

@richtabor would you be able to work on these 2 follow-ups:

  • updating the value of the COLORS.ui.borderFocus variable
  • checking all places in which the COLORS.ui.borderFocus variable is used throughout the codebase and make sure that the border styles are as per the latest spec

Happy to help with reviewing these changes.

@jasmussen
Copy link
Contributor

It looks like this PR achieves the desired 1.5px thickness by combining a 1px border with a 0.5px box-shadow — is that the expected "technique" for focus styles that should be applied to all components?

I don't know. I'm approving because the mix of focus styles we have now is very poor, and even a hacky solution is much much preferrable.

One nuance is that ideally we also have a consistent border radius; 2px in the resting state, and then a consistent radius for the focused state. Whether that's inwards, outwards, or centered, that we can figure out, but ideally that's the aspect that can be improved in next steps and potentially with consistency across.

@richtabor
Copy link
Member Author

@richtabor would be able to work on these 2 follow-ups:
updating the value of the COLORS.ui.borderFocus variable
checking all places in which the COLORS.ui.borderFocus variable is used throughout the codebase and make sure that the > border styles are as per the latest spec

Sure, I can do that in a follow-up PR today.

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

🚀

@ciampo ciampo merged commit 2b8afe0 into trunk Sep 13, 2023
50 checks passed
@ciampo ciampo deleted the fix/input-focus-shadow branch September 13, 2023 13:14
@github-actions github-actions bot added this to the Gutenberg 16.7 milestone Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants