CustomGradientPicker: Update Type and Angle controls #23802
Conversation
ItsJonQ
added
[Feature] UI Components
ItsJonQ
requested review from
ajitbohra,
chrisvanpatten,
ellatrix and
youknowriad
as code owners
|
Note: On the original issue, there were some interaction suggestions, such as...
from @pablohoneyhoney
from @jasmussen I took a look. It looks like it would involve some extensive refactors in the gradient slider portion of the component. For now, I think it would be easier (at least to start) to focus on the controls that appear on the bottom |
|
Size Change: +13.5 kB (1%) Total Size: 1.17 MB
ℹ️ View Unchanged
|
All good. I just saw some really sharp diagonal gradients that Kjell made, where there's no gradation because both colors are at the same position — so I ticketed it in #23816. Thank you for looking! Looking at this PR, here's a before:
And here's after:
This is night and day, it is profoundly better. It is such an improvement. 👏 Well done. There are a number of little followups I'd like to do separately, like putting all color swatches into a collapsible line line, revisit the clear buttons in a systematic way, and overall refine the sidebar. That is ongoing work, not forgotten:
... but something I will help with and which should not block this. In other words, this is good to ship, Q, just needs a code sanity check! 👍 👍 — Thanks again. |
ItsJonQ
force-pushed
the
update/gradient-controls
branch
from
a97c25e
to
af55622
Compare
@jasmussen Ah yes! I submitted a proposal to handle grouping/collapsing of like-controls: Thank you for your review + testing! |
|
@munirkamal Hiya! Thanks for reaching out! I'll reply in your issue 🙏 |
|
Love this 👏 |
There was a problem hiding this comment.
Cool stuff here @ItsJonQ ! 💯 - I've left some small comments but most importantly I'd like you to share your opinion about css-in-js.
I had to read a bit about this to review your code :), so I'm just trying to better understand the trade offs and I know you've taken a deep dive in styles.
My first concerns are:
- if we're adding a new way of styling (
emotioncss-in-js) and it's trade offs have been considered, is it the way to go for other components ( new ones or refactored) as well? - Should we use it only in complex scenarios? That means for example state based styles etc..
Thanks!
| @@ -0,0 +1,53 @@ | |||
| /** | |||
There was a problem hiding this comment.
Do you think we should have a new styles folder? Even though you're using css-in-js it's still the equivalent of style.scss which in many components I checked ( didn't check them all :) ) are in the root folder. So it seems to me we could keep it consistent. What do you think?
There was a problem hiding this comment.
Good point! It may no longer be necessary. I originally introduced this structure to make it distinctly different from the style.scss during the early proposal/experimental phase.
I'm happy to keep the style.js files at the root level. It's the same convention I've followed in another React component library project:
https://github.com/ItsJonQ/g2/tree/master/packages/components/src/Button
| @@ -0,0 +1,12 @@ | |||
| /** | |||
There was a problem hiding this comment.
Same comment as above about styles folder.
|
I just saw #23816 and was trying some things locally before coming here. |
|
@aristath Oo! Thanks for that suggestion. I just gave it a shot. It definitely allows for the points to be brought closer. However, the UI controls feel awfully cramped when this happens 🙈
This feels especially cramped when there's the (+) inserted that appears on hover:
@jasmussen What do you think? |
Here's what I see.
I LOVE the ability to make those sharp gradients. However in the above I actually set Which all is to say, I think it's a followup PR that should not block this PR from happening, and in that followup we could probably also make it so the plus button does not appear when you mouse-over one of the existing handles. Maybe? |
|
Great to see the progress here! |
|
Fixed size of bar to match mockups. |
|
Harmonized spacing, and improved focus.
|
|
Fixed so the swatches align on the right side with the gradient creator below.
|
There was a problem hiding this comment.
This is a vast improvement over what's shipping. And after the recent polish, this is good enough to ship from a design point of view.
I would love to get this in soon, so all it needs now is a final code review. @ntsekouras if you have the bandwidth, would appreciate you could take a look so we can get this in!
ZebulanStanphill
added
the
Needs Accessibility Feedback
There was a problem hiding this comment.
Great work here @ItsJonQ and the polishing from @jasmussen 💯
|
Should we move the "type" label to above now or later?
|
|
Thanks for the feedback + enhancements all! I'll merge this up 🎉 |
talldan
added
the
[Type] Breaking Change
This update improves the UI for the Linear/Gradient selector and angle controls for the
<CustomGradientPicker />component from@wordpress/components(which is featured in the Cover block).This helps partially resolve some of the suggestions proposed #21269 by folks like @pablohoneyhoney, @jasmussen, and @mapk.
Linear/Radial select
The internal
<SelectControl />has been updated with the new Gutenberg styles. The component functionality (that is the component API) remains the same. Since the UI of the new<SelectControl />matches that of<InputControl />, some of the internals were refactored so that they could be shared.Angle control
These have also received a UI update to match Gutenberg styles. The functionality remains mostly the same, with some minor improvements to the drag interaction. Also, the input that appears next to the circle is updated with our new
NumberControl.How has this been tested?
npm run devChecklist: