-
Notifications
You must be signed in to change notification settings - Fork 4.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
Image tools: Polish zoom slider #23418
Conversation
Size Change: +3.3 kB (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
@@ -259,6 +265,29 @@ export default function ImageEditor( { | |||
|
|||
return ( | |||
<> | |||
{ ! inProgress && ( | |||
<div | |||
className="richimage__zoom-control" |
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.
Are we keeping these "richimage" classes? They don't mean much.
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.
Happy to clean them up. Any preference, just "image__"?
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'd go with wp-block-image__
since the block class is wp-block-image
.
How is the arrow navigation fixed in the new secondary toolbar? |
It's not a toolbar, it's |
value={ Math.round( zoom ) } | ||
onChange={ setZoom } | ||
/> | ||
<ToolbarItem> |
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.
Shouldn't this only be used within a <Toolbar>
?
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 suppose yes, I'll look for an alternative.
To be honest, I'm not sure if I like a secondary "toolbar" (that looks like a toolbar but isn't). It's a bit confusing both for keyboard users and visually. |
As it is, there's simply too many confusing things shuffling around in the toolbar when you click crop, I would suggest that is already a problem both usability wise and keyboards wise. This is an improvement in that it reduces that, groups the cropping tools together, and visually simplifies. I still consider it a temporary solution until we can actually move the entire slider to the toolbar (#22569 (comment)). That, and remove the other unrelated buttons in the toolbar, until you press apply or cancel. |
I would love this to get unblocked and land, as tests are passing and it's a big improvement over what we had: a big bulky zoom slider popping out at the bottom. That same slider has been moved up and reduced, and it does not preclude further improvements. |
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.
Thank you ❤️
This PR polishes the image cropping zoom slider.
It:
I still think that it's worth moving the slider to the toolbar, but the efforts here will a) benefit that effort, and b) can serve as a holdover if we don't make that in time.
Thoughts?