Tooltip: add placement prop to replace deprecated position
#54264
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Flaky tests detected in c26f863. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6191995244
|
tyxla
reviewed
Sep 8, 2023
| if ( position !== undefined ) { | ||
| deprecated( '`position` prop in wp.components.tooltip', { | ||
| since: '6.4', | ||
| alternative: '`placement` prop', | ||
| } ); | ||
| } |
There was a problem hiding this comment.
We have this if ( position !== undefined ) condition above already, so perhaps we can just move the deprecated() call there, instead of duplicating the check?
There was a problem hiding this comment.
I borrowed the logic from the same changes made to URLPopover. Do you think it's worth following up there as well with your suggestions?
There was a problem hiding this comment.
Yes. Feel free to follow up in another PR, although it's not a huge deal, just a minor code style preference.
| } else if ( position !== undefined ) { | ||
| computedPlacement = positionToPlacement( position ); | ||
| } | ||
| computedPlacement = computedPlacement || DEFAULT_PLACEMENT; |
There was a problem hiding this comment.
I think we could just inline the default placement, no need to have a constant when it's only used once and in just a few lines below in the same function.
| @@ -35,12 +35,21 @@ The amount of time in milliseconds to wait before showing the tooltip. | |||
| - Required: No | |||
| - Default: `700` | |||
|
|
|||
| #### `placement`: `'top' | 'top-start' | 'top-end' | 'right' | 'right-start' | 'right-end' | 'bottom' | 'bottom-start' | 'bottom-end' | 'left' | 'left-start' | 'left-end'` | |||
|
|
|||
| Used to specify the tooltip's position with respect to its anchor. | |||
There was a problem hiding this comment.
We could add a reference to the placement docs:
There was a problem hiding this comment.
I grabbed this from Popover's README for consistency:
gutenberg/packages/components/src/popover/README.md
Lines 199 to 201 in ab97d28
So maybe we'd want to add the link there, too? And after mentioning URLPopover above, I looked at the README on placement:
gutenberg/packages/block-editor/src/components/url-popover/README.md
Lines 97 to 103 in ab97d28
So I wonder if we should follow up with these and other components moving towards placement.
There was a problem hiding this comment.
Sounds like a good documentation improvement 👍 If these are coming from an external library, then IMHO it makes a lot of sense to link to the original library's documentation.
There was a problem hiding this comment.
The counter-argument to that, is that we're exposing our internal implementation in our docs. We usually avoid doing this as it gives us more flexibility in changing underlying implementation (like when Popover was refactored from popper to floating-ui last year)
17 tasks
|
Size Change: -937 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
|
@tyxla @ciampo - I've updated some of the failing tests to include an expect warning assertion. There are two more failing tests which still need to be updated but:
diff --git a/packages/components/src/font-size-picker/test/index.tsx b/packages/components/src/font-size-picker/test/index.tsx
index 0d7816399b..b83a4a11be 100644
--- a/packages/components/src/font-size-picker/test/index.tsx
+++ b/packages/components/src/font-size-picker/test/index.tsx
@@ -36,32 +36,48 @@ describe( 'FontSizePicker', () => {
}
);
- test.each( [
- // Use units when first font size uses units.
- { firstFontSize: '12px', expectedValue: '80px' },
- // Don't use units when first font size does not use units.
- { firstFontSize: 12, expectedValue: 80 },
- ] )(
- 'should call onChange( $expectedValue ) after user types 80 when first font size is $firstFontSize',
- async ( { firstFontSize, expectedValue } ) => {
- const user = userEvent.setup();
- const onChange = jest.fn();
- render(
- <FontSizePicker
- __nextHasNoMarginBottom
- fontSizes={ [ { slug: 'slug', size: firstFontSize } ] }
- onChange={ onChange }
- />
- );
- await user.click(
- screen.getByRole( 'button', { name: 'Set custom size' } )
- );
- const input = screen.getByLabelText( 'Custom' );
- await user.type( input, '80' );
- expect( onChange ).toHaveBeenCalledTimes( 2 ); // Once per keystroke.
- expect( onChange ).toHaveBeenCalledWith( expectedValue );
- }
- );
+ test( 'should call onChange( 80px ) after user types 80 when first font size is 12px', async () => {
+ const user = userEvent.setup();
+ const onChange = jest.fn();
+ render(
+ <FontSizePicker
+ __nextHasNoMarginBottom
+ fontSizes={ [ { slug: 'slug', size: '12px' } ] }
+ onChange={ onChange }
+ />
+ );
+ await user.click(
+ screen.getByRole( 'button', { name: 'Set custom size' } )
+ );
+ const input = screen.getByLabelText( 'Custom' );
+
+ await user.type( input, '80' );
+
+ expect( console ).toHaveWarned();
+ expect( onChange ).toHaveBeenCalledTimes( 2 ); // Once per keystroke.
+ expect( onChange ).toHaveBeenCalledWith( '80px' );
+ } );
+
+ test( 'should call onChange( 80 ) after user types 80 when first font size is 12', async () => {
+ const user = userEvent.setup();
+ const onChange = jest.fn();
+ render(
+ <FontSizePicker
+ __nextHasNoMarginBottom
+ fontSizes={ [ { slug: 'slug', size: 12 } ] }
+ onChange={ onChange }
+ />
+ );
+ await user.click(
+ screen.getByRole( 'button', { name: 'Set custom size' } )
+ );
+ const input = screen.getByLabelText( 'Custom' );
+
+ await user.type( input, '80' );
+
+ expect( onChange ).toHaveBeenCalledTimes( 2 ); // Once per keystroke.
+ expect( onChange ).toHaveBeenCalledWith( 80 );
+ } );
describe( 'with > 5 homogeneous font sizes', () => {
const fontSizes = [But in diff --git a/packages/components/src/toggle-group-control/test/index.tsx b/packages/components/src/toggle-group-control/test/index.tsx
index 78be0e89fc..d685833628 100644
--- a/packages/components/src/toggle-group-control/test/index.tsx
+++ b/packages/components/src/toggle-group-control/test/index.tsx
@@ -98,6 +98,11 @@ describe.each( [
</Component>
);
+ if ( mode === 'uncontrolled' ) {
+ // eslint-disable-next-line jest/no-conditional-expect
+ expect( console ).toHaveWarned();
+ }
+
expect( container ).toMatchSnapshot();
} );
} );
|
|
This is what i'd do:
|
tyxla
reviewed
Sep 13, 2023
There was a problem hiding this comment.
Although I don't have strong feelings about it (we could alternatively follow up in additional PRs to replace all position with placement usages), I agree with the plan @ciampo proposed above, because that will help remove some of the warning inconsistencies we're seeing in the tests.
brookewp
force-pushed
the
tooltip/deprecate-position
branch
from
bd8e7fc to
97cf9ca
Compare
|
Thanks for the comments! I was hoping to replace internal usage in a second PR, but it seems cleaner to do it in this PR. That way we avoid adding warnings and rewriting tests temporarily to then be reverted in the follow-up PR. |
brookewp
force-pushed
the
tooltip/deprecate-position
branch
from
97cf9ca to
0ea691c
Compare
|
@ciampo I noticed in similar instances where we added |
The specific example of URLPopover didn't require a CHANGELOG entry because that component is not part of the
Since we're adding a new public API to the |
ciampo
changed the title
placement prop to replace positionplacement prop to replace deprecated position
ciampo
reviewed
Sep 14, 2023
There was a problem hiding this comment.
Left a couple of minor comments, but changes LGTM.
@chad1008 , would you be able to take a final look at this PR, including testing the updated usages of Tooltip across the block editor? Feel free to approve if things look good on your end
| @@ -225,6 +226,13 @@ export function UnforwardedButton( | |||
| </button> | |||
| ); | |||
|
|
|||
| // use floating-ui's `placement` instead of legacy `position` | |||
There was a problem hiding this comment.
Suggested change
| // use floating-ui's `placement` instead of legacy `position` | |
| // Convert legacy `position` values to be used with the new `placement` prop |
chad1008
approved these changes
Sep 14, 2023
There was a problem hiding this comment.
This looks good and tests well to me! I left one small comment of my own, and it looks like @ciampo may have added a couple thoughts on wording/exposing internal implementation details, but that's all code comment stuff that shouldn't be any kind of blocker 😄 🚀 🚢
| // Compute tooltip's placement: | ||
| // - give priority to `placement` prop, if defined | ||
| // - otherwise, compute it from the legacy `position` prop (if defined) | ||
| // - finally, fallback to the DEFAULT_PLACEMENT. |
There was a problem hiding this comment.
Now that we're inlining the default value, we can update this comment to say bottom instead of DEFAULT_PLACEMENT
@ciampo do you think it would make sense for |
|
Noting that something went off with the latest merge from trunk - there seem to be some extra changes that we likely don't want as part of the PR. |
Yup, sounds good to me.
Good catch. Let's fix that before merging. |
Great find! Thank you for looking out. 😄 I am trying to be friends with I'll get that resolved along with the conflict and update the changelog. |
This reverts commit 1f6fa73b93320c64671ad739a77116253a0b33fe.
brookewp
force-pushed
the
tooltip/deprecate-position
branch
from
c26f863 to
364d611
Compare
ciampo
approved these changes
Sep 18, 2023
tyxla
approved these changes
Sep 18, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #44401
What?
Similar to changes in Popover, the use of
floating-uiin Tooltip brings a new prop for positioning,placement.Why?
Currently, Tooltip uses the
positionprop, which we will be replacing withplacement. This starts the deprecation process so we can slowly introduceplacementto avoid breaking changes.How?
By exposing
placement, updating the logic to favorplacementoverposition, adding deprecation warnings, updating types and the README. As well as updating internal uses ofpositiontoplacement.Testing Instructions
Ensure unit tests are passing and don't show unexpected
console.warnerrorsSmoke tests
npm run storybook:devTooltip
positionfor tooltip and check that the error appears in the consoleplacementto ensure it takes precedence overposition.Button
showTooltipand add alabel.bottom.tooltipPositionto other options to ensure the tooltip displays where expected.Others
positionwas changed toplacement. Ensure that results match trunk. I highlight this because some positions appear incorrect, but the behavior is the same. i.e.ToggleGroupControlWhat
position: toplooks like in trunk:And if we experiment and change to
bottom right:Something strange is happening here, so I'll add a to-do item for it.
BoxControl* (unlinked side units) andDateTimePicker(UTC text) can be tested in storybook.*Side note: Will also add a to-do follow up for
BoxControl:First, in the screenshot above, the tooltip overlaps, which doesn't seem ideal. But also, a custom implementation of Tooltip has been added as
BaseTooltipwhich I believe is worth looking into.