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.

Sign up for GitHub

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: Align tooltip with others element spacing wise #64454

Open
wants to merge 11 commits into
base: trunk
Choose a base branch
from
Open

Fix: Align tooltip with others element spacing wise #64454

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
7ca74aa
alter styles of move up and move down label
Rishit30G Aug 12, 2024
6762d98
Function to get direction using direction and orientation
Rishit30G Aug 13, 2024
3064043
Add Tooltip direction
Rishit30G Aug 13, 2024
6d5fd4c
remove redundant styles
Rishit30G Aug 13, 2024
20b6b73
Alter gutter based on direction
Rishit30G Aug 13, 2024
c3b3004
fix tooltipDirection types
Rishit30G Aug 13, 2024
d29cce4
fix block-mover height style
Rishit30G Aug 14, 2024
ff1030a
Update tooltipDirection with existing tooltipPosition
Rishit30G Aug 14, 2024
169504b
Update gutter value based on direction
Rishit30G Aug 14, 2024
10aaa2e
revert grid-unit-05 style
Rishit30G Aug 15, 2024
c6be2d3
Refactor block-mover button and tooltip gutter value
Rishit30G Aug 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions packages/block-editor/src/components/block-mover/button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,21 @@ const getArrowIcon = ( direction, orientation ) => {
return null;
};

const getDirection = ( direction, orientation ) => {
if ( direction === 'up' ) {
if ( orientation === 'horizontal' ) {
return isRTL() ? 'right' : 'left';
}
return 'up';
} else if ( direction === 'down' ) {
if ( orientation === 'horizontal' ) {
return isRTL() ? 'left' : 'right';
}
return 'down';
}
return null;
};

const getMovementDirectionLabel = ( moveDirection, orientation ) => {
if ( moveDirection === 'up' ) {
if ( orientation === 'horizontal' ) {
Expand Down Expand Up @@ -139,6 +154,7 @@ const BlockMoverButton = forwardRef(
direction,
orientation
) }
tooltipDirection={ getDirection( direction, orientation ) }
aria-describedby={ descriptionId }
{ ...props }
onClick={ isDisabled ? null : onClick }
Expand Down
2 changes: 2 additions & 0 deletions packages/components/src/button/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ export function UnforwardedButton(
children,
size = 'default',
text,
tooltipDirection,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Toolbar button metrics

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Screenshot 2024-08-14 at 10 14 22 AM

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Rishit30G Rishit30G Aug 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed ✅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🙏

variant,
description,
...buttonOrAnchorProps
Expand Down Expand Up @@ -272,6 +273,7 @@ export function UnforwardedButton(
tooltipPosition &&
// Convert legacy `position` values to be used with the new `placement` prop
positionToPlacement( tooltipPosition ),
direction: tooltipDirection,
}
: {};

Expand Down
4 changes: 4 additions & 0 deletions packages/components/src/button/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,10 @@ type BaseButtonProps = {
* 'link' (the link button styles)
*/
variant?: 'primary' | 'secondary' | 'tertiary' | 'link';
/**
* If provided, renders the direction of the tooltip.
*/
tooltipDirection?: 'up' | 'down' | 'right' | 'left';
};

type _ButtonProps = {
Expand Down
14 changes: 12 additions & 2 deletions packages/components/src/tooltip/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ function UnforwardedTooltip(
position,
shortcut,
text,

direction,
...restProps
} = props;

Expand Down Expand Up @@ -93,6 +93,16 @@ function UnforwardedTooltip(
placement: computedPlacement,
showTimeout: delay,
} );

const getGutterValue = () => {
if ( direction === 'up' ) {
return 28;
} else if ( direction === 'down' ) {
return 8;
}
return 4;
};

const mounted = tooltipStore.useState( 'mounted' );

if ( isNestedInTooltip ) {
Expand Down Expand Up @@ -130,7 +140,7 @@ function UnforwardedTooltip(
{ ...restProps }
className={ clsx( 'components-tooltip', className ) }
unmountOnHide
gutter={ 4 }
gutter={ getGutterValue() }
id={ describedById }
overflowPadding={ 0.5 }
store={ tooltipStore }
Expand Down
1 change: 0 additions & 1 deletion packages/components/src/tooltip/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,3 @@
.components-tooltip__shortcut {
margin-left: $grid-unit-10;
}

4 changes: 4 additions & 0 deletions packages/components/src/tooltip/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,10 @@ export type TooltipProps = {
* The text shown in the tooltip when anchor element is focused or hovered.
*/
text?: string;
/**
* The direction of the tooltip arrow.
*/
direction?: string;
};

export type TooltipInternalContext = {
Expand Down
Loading