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 CSS styles of ColorPicker #18448

Merged
merged 1 commit into from Nov 28, 2019
Merged

Conversation

@enriquesanchez
Copy link
Contributor

enriquesanchez commented Nov 11, 2019

Description

Resolves #18129

This PR attempts to polish the styling of the ColorPicker component when used outside of WordPress.

Screenshots

Before:

67658418-cd1ac500-f959-11e9-90c7-c1983c38818a
67658419-cd1ac500-f959-11e9-843d-d77ffff284c9

After:

2019-11-11 16-25-04 2019-11-11 16_25_38

Types of changes

CSS and visual.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .
@enriquesanchez

This comment has been minimized.

Copy link
Contributor Author

enriquesanchez commented Nov 11, 2019

Question for reviewers: how much overriding of default styles can or should I be doing? For example, I'd love if the toggle button's height was the same as the input fields, but the button's styles are declared elsewhere.

Screen Shot 2019-11-11 at 16 34 20

Copy link
Contributor

youknowriad left a comment

how much overriding of default styles can or should I be doing? For example, I'd love if the toggle button's height was the same as the input fields, but the button's styles are declared elsewhere.

I think it's fine to override button styles to achieve this.

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Nov 12, 2019

Yep!

Riad's comments are ones I would have made, so plus one to that! Nice work.

@enriquesanchez

This comment has been minimized.

Copy link
Contributor Author

enriquesanchez commented Nov 12, 2019

Updated the button's padding to match the height of the inputs (thanks @youknowriad and @jasmussen for the feedback).

2019-11-12 13-44-16 2019-11-12 13_44_49

Copy link
Contributor

jasmussen left a comment

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 13, 2019

Is it just me or the height of the button is not the same as the input in the Gutenberg context
Capture d’écran 2019-11-13 à 11 59 37 AM

@richtabor

This comment has been minimized.

Copy link
Contributor

richtabor commented Nov 13, 2019

The icon looks like a dropdown, but isn't a dropdown. It's not critical we fix this because the alternatives aren't that great, but here are some thoughts for alternative icons:

Agree 100%. I assumed this was a dropdown or a panel opening of some sort.

@richtabor

This comment has been minimized.

Copy link
Contributor

richtabor commented Nov 13, 2019

Perhaps this should be new issue/idea, but what about a persistent ButtonGroup, instead of a toggle to view the next state (which is unknown to the user):

Screen Shot 2019-11-13 at 11 43 37 AM

@ItsJonQ

This comment has been minimized.

Copy link
Contributor

ItsJonQ commented Nov 14, 2019

Perhaps this should be new issue/idea, but what about a persistent ButtonGroup, instead of a toggle to view the next state (which is unknown to the user):

@richtabor Ooo. That's a great suggestion. Wow, I too thought it was a dropdown 🙃 . I think the use of a ButtonGroup would be much clearer in this case

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Nov 14, 2019

Agreed on the buttongroup. But definitely worth exploring as a subsequent PR, because even just this PR alone makes things better 🧡

@enriquesanchez

This comment has been minimized.

Copy link
Contributor Author

enriquesanchez commented Nov 21, 2019

Another alternative to try is to make this an actual dropdown. Here's how Figma does it for inspiration:

68341137-6ad66880-00ad-11ea-8af0-867a4fe0298c

68341197-92c5cc00-00ad-11ea-9ea0-b5d8171b3776

Screen Shot 2019-11-20 at 18 27 05

@jasmussen @ItsJonQ @richtabor what do you think?

@jasmussen

This comment has been minimized.

Copy link
Contributor

jasmussen commented Nov 21, 2019

I think it could absolutely be either a dropdown or a button group. But I also think it's worth doing that separately, just so the great improvements of this PR can ship sooner ;)

Copy link
Contributor

ItsJonQ left a comment

@enriquesanchez 🎉 Thank you for improving this UI!

I've attempted to restart the Travis tests. They were failing at the E2E parts. I don't think it was anything this PR did. Hopefully, it'll resolve itself this time around

I think it could absolutely be either a dropdown or a button group. But I also think it's worth doing that separately, just so the great improvements of this PR can ship sooner ;)

I agree with @jasmussen .

@enriquesanchez enriquesanchez force-pushed the try/better-styles-for-ColorPicker branch from 4c06f1b to 18711c1 Nov 21, 2019
@mapk
mapk approved these changes Nov 21, 2019
Copy link
Contributor

mapk left a comment

Yep, the changes are a definitely improvement. 👍 Would you mind opening up another PR or issue that addresses the other concerns in this thread, @enriquesanchez, if it hasn't been done already? Thank you!

@enriquesanchez

This comment has been minimized.

Copy link
Contributor Author

enriquesanchez commented Nov 21, 2019

I've created #18678 to explore improvements to the color format toggle 👍

@enriquesanchez enriquesanchez force-pushed the try/better-styles-for-ColorPicker branch from 18711c1 to 70d0910 Nov 26, 2019
@enriquesanchez

This comment has been minimized.

Copy link
Contributor Author

enriquesanchez commented Nov 28, 2019

@youknowriad would you be able to help me get this PR to pass Travis CI tests? I've tried rebasing and re-unning the tests a few times and it's still stuck :(

@youknowriad youknowriad force-pushed the try/better-styles-for-ColorPicker branch from 70d0910 to 78e4a63 Nov 28, 2019
@enriquesanchez

This comment has been minimized.

Copy link
Contributor Author

enriquesanchez commented Nov 28, 2019

Thanks @youknowriad! 🙏

@enriquesanchez enriquesanchez merged commit c65477e into master Nov 28, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@enriquesanchez enriquesanchez deleted the try/better-styles-for-ColorPicker branch Nov 28, 2019
@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 28, 2019

I was just lucky :p rebased again. I think folks worked on making the tests more stable lately.

@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.