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

Update border and focus style of the Input selector in ColorPicker Component #50703

Merged
merged 5 commits into from
May 22, 2023

Conversation

falgunihdesai
Copy link
Contributor

Adds backdrop to the color value type input selector in the ColorPicker component. Previously, the backdrop for this component was hidden.

Fixes #50524

What?

Why?

How?

Testing Instructions

Run "npm storybook:dev" and select ColorPicker component. Focus the navigation tab on Color Value type dropdown selector

Testing Instructions for Keyboard

Open the Color Picker in Gutenberg editor and focus on the Color Value type dropdown selector using tab key

Screenshots or screencast

Screenshot 2023-05-17 at 6 42 09 PM

Adds backdrop to the color value type input selector in the ColorPicker component.
Previously, the backdrop for this component was hidden. Fixes WordPress#50524
@falgunihdesai falgunihdesai requested a review from ajitbohra as a code owner May 17, 2023 14:43
@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label May 17, 2023
@github-actions
Copy link

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @falgunihdesai! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

Copy link
Contributor

@mcsf mcsf left a comment

Choose a reason for hiding this comment

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

Hi, thanks for your patch!

I left a question about the choice of display: flex, but otherwise this looks good to me.

I've attached a screen recording clearly showing the focus on Hex select control.

gh-50703-testing.mov

@@ -30,7 +30,7 @@ export const SelectControl = styled( InnerSelectControl )`
margin-left: ${ space( -2 ) };
width: 5em;
${ BackdropUI } {
display: none;
display: flex;
Copy link
Contributor

Choose a reason for hiding this comment

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

Was there a reason to prefer flex over block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used flex because other elements like Input box for color value and copy button both uses flex. Otherwise block was also working 😄. Was one of the two failed checks because of flex. Failure of Changelog diff check I understood. But the reason for another failed check is not clear to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't see it, but if you saw another failure it may have been a flaky end-to-end test. Everything looks good now, so I'll merge. :)

Copy link
Contributor

@mcsf mcsf left a comment

Choose a reason for hiding this comment

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

I took the liberty of changing the display to block and added an entry to the corresponding change log. :)

@mcsf mcsf merged commit 8b12c1c into WordPress:trunk May 22, 2023
@github-actions
Copy link

Congratulations on your first merged pull request, @falgunihdesai! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

@github-actions github-actions bot added this to the Gutenberg 15.9 milestone May 22, 2023
@mcsf
Copy link
Contributor

mcsf commented May 22, 2023

Thanks for finding the issue and fixing it, @falgunihdesai !

@falgunihdesai
Copy link
Contributor Author

Thank you for the help @mcsf .

@ciampo
Copy link
Contributor

ciampo commented May 22, 2023

Just flagging that another PR (#50609) was also opened to fix this, in which we evaluated trade-offs between short-term fixes and design requirements

@mcsf
Copy link
Contributor

mcsf commented May 22, 2023

Thanks for the heads-up, @ciampo! Feel free to override this in #50609, I trust everyone's judgement there.

@bph bph added the [Package] Components /packages/components label May 24, 2023
@cbravobernal cbravobernal added the [Type] Bug An existing feature does not function as intended label May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [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.

ColorPicker: Color space dropdown has no focus style
5 participants