Fix: Align tooltip with others element spacing wise

[Rishit30G]

What?

This PR is intended to fix: #64440

How?

Added specific styles to Move Up and Move Down Labels

Testing Instructions

• Create a new post
• Add a paragraph
• Notice the direction button label spacing from the Toobar

Screenshots or screencast

Screen.Recording.2024-08-12.at.1.25.47.PM.mov

[github-actions]

Warning: Type of PR label mismatch

To merge this PR, it requires exactly 1 label indicating the type of PR. Other labels are optional and not being checked here.

• Type-related labels to choose from: [Type] Automated Testing, [Type] Breaking Change, [Type] Bug, [Type] Build Tooling, [Type] Code Quality, [Type] Copy, [Type] Developer Documentation, [Type] Enhancement, [Type] Experimental, [Type] Feature, [Type] New API, [Type] Task, [Type] Technical Prototype, [Type] Performance, [Type] Project Management, [Type] Regression, [Type] Security, [Type] WP Core Ticket, Backport from WordPress Core.
• Labels found: .

Read more about Type labels in Gutenberg. Don't worry if you don't have the required permissions to add labels; the PR reviewer should be able to help with the task.

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: Rishit30G <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Co-authored-by: tyxla <[email protected]>
Co-authored-by: mirka <[email protected]>
Co-authored-by: hbhalodia <[email protected]>
Co-authored-by: ciampo <[email protected]>
Co-authored-by: gigitux <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[t-hamano]

Thanks for the PR!

I think adding styles to the Tooltip component that depend on specific text should be avoided for the following reasons:

• The Tooltip component should not depend on a specific context
• The text may be localized and changed to a different text
• It also affects other tooltips that contain text such as up and down
• The tooltip is not guaranteed to appear at the bottom

The current Tooltip component has a hardcoded gutter. My thought is that there may need to be a prop like offset or tooltipOffest to modify this gutter in some way, like in the Popover component.

cc @WordPress/gutenberg-components

[Rishit30G]

Hey @t-hamano
Thanks for reviewing the PR and sharing your valuable feedback, agree with all your points 🙇
My intension was not to raise this PR but keep it as draft so that today I could have researched more on the caveats of my approach ( my bad ) 😅
Nonetheless, I'll try to work on the approach you have shared 👍
Thanks

[tyxla]

Unfortunately, I don't think this is the way to go.

I'd recommend trying to adjust the component that consumes the Tooltip, because this is a particular scenario related to a very specific use case.

Furthermore, if we were to add spacing to Tooltips, I'd expect we do it in a consistent way that applies to all of them.

[Rishit30G]

Thanks @tyxla for the review
I agree with all your points, they are good insights for me to understand my mistake
Will try to find a workaround for the same as per your suggestions 👍

[tyxla]

Thanks for your hard work on it @Rishit30G 🙌

[Rishit30G]

Updates on PR as per feedback 📝

• Updated the Button component with a new prop tooltipDirection
• tooltipDirection can get a value of up | down | right | left based on the direction
• Made a function getDirection which takes in two parameters direction and orientation which will help us avoid the problem of changing styles of right and left chevron labels when we only want to update up and down chevrons.
• Updated the Tooltip with new prop direction (string) which will capture the value of tooltipDirection
• Used the direction prop in Tooltip component to set the gutter value for appropriate spacing

cc: @tyxla @t-hamano

Sharing the screencast for the same:

Screen.Recording.2024-08-13.at.12.40.15.PM.mov

[mirka]

Note that there is already a tooltipPosition prop on Button that can you can use to change the tooltip placement (e.g. "top" and "bottom").

I don't think we will need any changes in the @wordpress/components package to land this PR. As for fine-tuning the "bottom" placement so the block mover down tooltip aligns with the other toolbar button tooltips, I think a cleaner strategy would be to fix the metrics of the block mover buttons so their vertical edges are aligned with the other buttons. If you add a visible outline to the buttons, you can see that the block mover buttons are shorter. This is causing the misalignment of the tooltips.

[hbhalodia]

Hi @mirka, Agree on this, We just need to adjust this style added here which make this height -

gutenberg/packages/block-editor/src/components/block-mover/style.scss

Line 12 in 46f9fe0

height: $block-toolbar-height * 0.5 - $grid-unit-05;

as 24px, by removing the $grid-unit-05, not sure why this added, may be to decrease the space between chevrons,

Also, need to adjust the top and bottom padding for up and down button to 10px, so that it looks what it looked like earlier.

@Rishit30G, Could you please adjust this style and check if it helps,

The move up, would still not in place, but move down would be consistent with different toolbar tooltips.

[tyxla]

Agreed @mirka's suggestions are the way to go. We should use what we have and keep any specific customization outside of the Tooltip component.

[Rishit30G]

Thanks for the suggestions @mirka, @hbhalodia, @tyxla 🙇

• I have removed extra tooltipDirection and used tooltipPosition
• Updated the style from height: $block-toolbar-height * 0.5 - $grid-unit-05; to height: $block-toolbar-height * 0.5;

[t-hamano]

the "$grid-unit-05", not sure why this added

This style was added when introducing a custom scrollbar to the block toolbar in #57444 (Sorry for not leaving a comment 🙇‍♂️).

If we remove $grid-unit-05, an unnatural shift will occur when the mover button is focused on Windows OS:

500cb196af93dc4912fdc5a684188eac.mp4

We tried a lot of different things to fix this problem in #57444, but for now, I think it's better not to remove $grid-unit-05.

Of course, if the top toolbar is disabled, there's no problem with removing $grid-unit-05, but if adjusting the tooltip position becomes complicated, I think it's better to leave $grid-unit-05.

[Rishit30G]

Thanks for the info @t-hamano! 🙌🏻
I already made a workaround for this in previous commits, will see if I can incorporate it without breaking

[Rishit30G]

Fixed ✅

[ciampo]

Hey @Rishit30G , I agree with @mirka 's suggestions above, especially about

I don't think we will need any changes in the @wordpress/components package to land this PR

Can we make sure this PR doesn't include changes to Button and Tooltip ? Thank you 🙏

[Rishit30G]

Thanks for sharing the inputs! 🙇
Sorry for the late response, I was on a break from last Thursday.
Currently occupied at workplace, will resume my work on this in a while