Spacer: Show tooltip with height value on resize #23077
Conversation
Also adjust properties with axis and isEnabled properties
ItsJonQ
added
[Type] Enhancement
ItsJonQ
requested review from
ajitbohra,
chrisvanpatten,
Soean,
talldan and
youknowriad
as code owners
| const classes = classnames( 'components-resizable-visualizer', className ); | ||
|
|
||
| return ( | ||
| <Root aria-hidden="true" className={ classes } ref={ ref } { ...props }> |
There was a problem hiding this comment.
By default, I felt like it may be better to hide this tooltip via aria-hidden. This can be overridden via incoming props.
|
Size Change: +1.77 kB (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
| <Root aria-hidden="true" className={ classes } ref={ ref } { ...props }> | ||
| { resizeListener } | ||
| <Label | ||
| aria-hidden={ props[ 'aria-hidden' ] } |
There was a problem hiding this comment.
We have to pass this through manually to apply to the inner label element. The reason is because the inner label is Portal'ed, which means it will not inherit the aria-hidden attribute from the <Root /> component.
|
This is awesome. I notice there is some strange behavior on FF. It's not sticking to the cursor. It seems the position is only updating when I pause for a second.
As far as the design and functionality, this is pretty great. I think sticking it to the cursor more useful because my eye is already looking there. Once that minor bug is fixed, I can see it being useful for all sorts of things. Nice work. |
|
@MichaelArestad Oooo! Thank you for verifying!! I'll take a look on Firefox 🦊 |
|
@MichaelArestad Can confirm your experience. Hmm! It's so weird... On Firefox, the tooltip can follow my mouse just fine. However, when I drag to resize (using the It may be something funky in that |
Funky. Thanks for confirming. |
|
@MichaelArestad I think I did it! Made some minor tweaks.. The biggest one was binding the |
@ItsJonQ Nice! Can confirm! However, I have a new bug for you. I have to click the spacer drag handle twice before it the measurement shows up. I have to do this for every spacer I add to the same document. |
|
@MichaelArestad Thank you!! @mcsf 's feedback helped me rethink + optimize things 😉 |
| document.removeEventListener( 'mousedown', handleOnMouseDown ); | ||
| document.removeEventListener( 'mousemove', handleOnMouseMove ); | ||
| document.removeEventListener( 'mouseup', handleOnMouseUp ); |
There was a problem hiding this comment.
Just looking at the code, I don't think this cleanup ever does anything. That's because the
handle*functions are re-instantiated every time the effect runs.
I don't why I said this, as it didn't make sense: the registered listeners are properly referenced in the tear-down function returned by the effect, so we should always be able to remove the old event listeners.
| document.removeEventListener( 'mousedown', handleOnMouseDown ); | ||
| document.removeEventListener( 'mousemove', handleOnMouseMove ); | ||
| document.removeEventListener( 'mouseup', handleOnMouseUp ); |
There was a problem hiding this comment.
Let me know what you think of these revisions
So I've been going back and forth looking at this effect and feeling generally unsatisfied when pondering things like the dependencies, and I think I know why. We're dealing with effects of first degree and effects of second degree, so to speak: the first-degree effects are the registration of the mouse* event listeners; the second-degree ones are the listeners themselves. This makes the reasoning about dependencies more confusing.
If we were stricter about constructing these effects, we might refactor this as follows:
export function useResizeLabel( … ) {
// ...
const clearMoveTimeout = useCallback( () => {
if ( moveTimeoutRef.current ) {
clearTimeout( moveTimeoutRef.current );
}
}, [ moveTimeoutRef ] );
const handleOnMouseDown = useCallback( () => {
if ( ! isCursor ) return;
setIsDragging( true );
clearMoveTimeout();
unsetMoveXY();
}, [
// meaningful dependencies (mutable)
isCursor,
// formal dependencies (assumed constant)
setIsDragging, clearMoveTimeout, unsetMoveYX,
] );
const handleOnMouseMove = useCallback( /* same as above */ );
const handleOnMouseUp = useCallback( /* same as above */ );
useEffect( () => {
document.addEventListener( 'mousedown', handleOnMouseDown );
return () => {
document.removeEventListener( 'mousedown', handleOnMouseDown );
};
}, [ handleOnMouseDown ] );
useEffect( /* same as above for `mousemove` */ );
useEffect( /* same as above for `mouseup` */ );
return { ... };
}Note those comments about "meaningful" vs. "formal" dependencies. That was my of not making a decision as to whether we should be a strict as possible when listing dependencies or not. :) I'll leave that up to you.
|
Love this. I do wonder about the capitalization of "PX". Maybe its just the size of that unit's label, but I wonder if lowercase ("px") would be better? I guess its mostly personal preference. |
|
@shaunandrews + @MichaelArestad Lowecase
Vote with 👍 / 👎 Thank you! |
|
Thanks @MichaelArestad ! Lowercase it is :) |
|
Do we use units like this elsewhere? What does padding do? |
There was a problem hiding this comment.
Looking at ResizeTooltop, what if we didn't have cursor as a possible position? Might not we get by with either bottom or corner?
I ask for a couple of reasons. I think there is a difficult balance to find between working "one need at a time", and building components that aim to be framework-ready (customisable, reusable) from the start. My personal impression from this PR is that we may be leaning too much towards the latter. I'd like to start off with less: fewer modes, fewer overrides* (e.g. do we need to pass zIndex? maybe we do, but I'd like to know the reasoning).
The other thing is that it would be good to have a baseline — in terms of UX, of performance, of any such criterion — against which to measure any improvements. In this case, we would err on the side of simplicity, both in the complexity of the implementation and in the richness of the interactions, by only adding one or two of the tool-tip positioning modes. That way, any subsequent PR to improve or expand would give us much more insight on what works best.
*: I always worry about options and overrides because they become interfaces to mind when thinking about backwards compatibility in the present and in the future.
|
@mcsf Those are fair points. I've removed the "cursor" position from the ResizeTooltip. I've adjusted some logic and styles to improve the integration of We can revisit adding a cursor positioned Tooltip in the future, if needed 👍 |
Thanks for humouring me! |
Anytime 😉 ❤️ |
There was a problem hiding this comment.
Everything is very detailed and carefully laid out, and I appreciate that.
I did have a question about how to fit ResizeTooltop in ResizableBox, though (the position I tend to come from is to minimise how much is exposed at first, and see over time what should be opened up).
| <ResizeTooltip | ||
| axis="y" | ||
| isVisible={ isResizing } | ||
| position="bottom" | ||
| /> |
There was a problem hiding this comment.
Have you considered rendering actually in ResizableBox? If there are reasons to keep it separate, do let me know. Otherwise, it might be a boon in developer experience that ResizableBox could handle everything related to resizing on its own.
I can't help but think that axis and position can be derived from ResizableBox's enable and to some extent size props. If we were to move Tooltip into Box the consumer of the component wouldn't have to worry about such extra props.
There was a problem hiding this comment.
@mcsf I have considered putting it in ResizableBox :). That's how this idea started out.
The reason I thought of keeping it independent was for added flexibility. That is, to show dimensions on resize that aren't triggered by ResizableBox. For example, browser window resize.
To start, we could have this be a default (experimental) feature of ResizableBox. Something like <ResizableBox __experimentalShowTooltip={true} /> would show the Tooltip.
What do you think? :)
There was a problem hiding this comment.
The reason I thought of keeping it independent was for added flexibility. That is, to show dimensions on resize that aren't triggered by
ResizableBox. For example, browserwindowresize.
Hm, I could see that as an interaction, although personally I would wonder when we actually expect users to resize their proper window in a context where we'd show the tool tip. :)
So I think we're still in this territory: "the position I tend to come from is to minimise how much is exposed at first, and see over time what should be opened up". Internally, ResizeTooltip is a good component to build separately, but I would keep it private to our components.
To start, we could have this be a default (experimental) feature of
ResizableBox. Something like<ResizableBox __experimentalShowTooltip={true} />would show the Tooltip.
I'd be happy to see that experiment. :) Don't feel like you need to do it in this PR, though, as long as we keep the right things private/experimental.
There was a problem hiding this comment.
Internally, ResizeTooltip is a good component to build separately, but I would keep it private to our components.
@mcsf Sounds good to me 👍 . With this PR, in it's current state, does exporting it as __experimental qualify? Or would it be better to integrate it into ResizableBox and not export it at all?
Thanks!
There was a problem hiding this comment.
in it's current state, does exporting it as
__experimentalqualify? Or would it be better to integrate it intoResizableBoxand not export it at all?
If you don't mind too much, integrating in ResizableBox and not exporting would be the best, IMO!
There was a problem hiding this comment.
@mcsf Alrighty! Let's go with that for now :)
|
@mcsf I just moved the ResizeTooltip under ResizableBox. Lemme know if this works! <3 |
| @@ -1022,7 +1022,7 @@ | |||
| { | |||
| "title": "ResizeTooltip", | |||
| "slug": "resize-tooltip", | |||
| "markdown_source": "../packages/components/src/resize-tooltip/README.md", | |||
| "markdown_source": "../packages/components/src/resizable-box/resize-tooltip/README.md", | |||
There was a problem hiding this comment.
Should no longer be in the manifest, right?
There was a problem hiding this comment.
I'm... not sure. This was auto generated. There's a README.md inside the directory. I guess I can remove it? Ideally I'd like to preserve the docs though.
An alternative would be to inline comment some of the documentation (for now)
There was a problem hiding this comment.
Hmm... yeah, I see now. Well, I don’t have a preference. Maybe @nosolosw has a different take on how we should document components and let these tools work for us? Anyway, LGTM :)
There was a problem hiding this comment.
I took a peek at how the script that processes the handbook docs works. It seems that it looks for README files within the components folder (see) and then updates the manifest used to expose them in the handbook (see ResizableBox page).
I'm unfamiliar with how we deal with subcomponents that aren't exposed via the @wordpress/components package. I had a quick look and this seems to be the only case? Anyway, it seems that we should either expose the resize-tooltip as a component proper or look at ways to remove its page from the handbook as it can't be used by external consumers of the API. For the latter, we could go with a quick hack (use a lowercased readme, use any other name for the file) or fix the issue (update the glob pattern to only match README files within the top-level component folder).
|
Alrighty! Travis is green and happy. Thanks for the review @mcsf !! I'll merge it up |
|
🎉 |
Description
This update enhances the Spacer block by rendering a tooltip element above the cursor when the user engages in a "drag to resize" interaction.
✨ New
<ResizeTooltip />componentThe tooltip comes from a new
<ResizeTooltip />component. This component is a general resize-aware component that renders a tooltip on size changes. It isn't bound to<ResizableBox />(which is what Spacer uses), but it can be integrated with it.As an aside... This component has 2 different positions:
bottomcorner(default)📄 Corner
The Tooltip will render on the top/right of the parent element.
⬇️ Bottom
The Tooltip will render on the bottom/center of the parent element.
🎨 Design
As far as the Tooltip design goes... I went with a UI that was in the spirit of "G2". Something minimal, sporting the all black/white look.
How has this been tested?
To test...
npm installnpm run devTypes of changes
react-resize-awareas dependency in@wordpress/components(it's also used in@wrodpress/compose)<ResizeTooltip />componentResizeTooltipfrom@wordpress/components<ResizeTooltip />into Spacer blockChecklist:
The idea for this PR originally came from:
#22956