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.

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

Added missing left and right placement positions #52149

Closed
wants to merge 1 commit into from

Conversation

bfintal
Copy link
Contributor

@bfintal bfintal commented Jun 30, 2023

What?

Added left and right position definitions.

Why?

I noticed that when I used tooltipPosition="right" and tooltipPosition="left" in a Button component, it didn't work. However, "top" and "bottom" both work. To make right and left work, you'll need to add middle y axis value to it: "middle right" and "middle left".

How?

It looked like it didn't work because the definition for "right" and "left" were not defined in the list of shorthand positions. Since "top" and "bottom" values worked, I think it should be that "right" and "left" values should be added in as well.

Testing Instructions

Add a Button that uses "right" or "left" tooltipPositions:

// This now works:
<Button label="Tooltip" tooltipPosition="right">Hover me</Button>
// The above is identical to:
<Button label="Tooltip" tooltipPosition="middle right">Hover me</Button>

// And this now works:
<Button label="Tooltip" tooltipPosition="left">Hover me</Button>
// Which is identical to:
<Button label="Tooltip" tooltipPosition="middle left">Hover me</Button>

Testing Instructions for Keyboard

Screenshots or screencast

@bfintal bfintal requested a review from ajitbohra as a code owner June 30, 2023 04:37
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

It looks like we have some typescript errors related to this update. I guess we need to update the records in other places?

cc: @ciampo In case he has some thoughts on supporting left and right shorthand placements.

@ciampo
Copy link
Contributor

ciampo commented Sep 13, 2023

Thank you for the pint, @jorgefilipecosta !

Hey @bfintal , unfortunately the left and right values are not supported by the component — that is because these are the same values supported by the position prop of the Popover component, which was originally determined by the underlying third-party library that was used when the component was created originally. You can see the list of supported values for the tooltipPosition prop on the Storybook docs page.

Furthermore, we are slowly migrating away from the position prop in our components — in fact, we are just in the process of marking it as deprecated on the Tooltip component, and other components like Button will follow. The position prop will be replaced by a new placement prop, which better aligns with how the latest version of Popover works.

For the reasons stated above, I'm going to close this PR — the easy fix is to use "middle right" and "middle left" as mentioned in the PR description.

Thank you!

@ciampo ciampo closed this Sep 13, 2023
@bfintal bfintal deleted the patch-5 branch September 22, 2023 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants