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

feat(tooltip): Add optional prop to auto orientate within parent bounds #9556

Merged

Conversation

TuxedoFish
Copy link
Contributor

Closes #7321

  • Tooltip is escaping from the screen when label is close to top bottom left or right borders
  • Adds autoOrientation prop which allows the align and direction props to be updated

Changelog

New

  • Adds in an AutoOrientation story to the storybook for the Tooltip
  • Adds in getBestOrientation function to FloatingMenu
  • Adds in autoOrientation prop to the Tooltip which uses this function

Changed

  • direction and align are initially set via props but the rendered menu uses state

Removed

  • N/A

Testing / Reviewing

  • Turning off autoOrientation should have the existing behaviour of tooltips falling outside of the container bounds
  • Turning on autoOrientation should stop the tooltip from being able to fall outside of the bounds of its container

@netlify
Copy link

netlify bot commented Aug 28, 2021

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: debe2a3

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/6160769758a83d00087336bb

😎 Browse the preview: https://deploy-preview-9556--carbon-react-next.netlify.app

@github-actions
Copy link
Contributor

github-actions bot commented Aug 28, 2021

DCO Assistant Lite bot All contributors have signed the DCO.

@TuxedoFish
Copy link
Contributor Author

I have read the DCO document and I hereby sign the DCO.

1 similar comment
@Harry-Liversedge
Copy link
Contributor

I have read the DCO document and I hereby sign the DCO.

@netlify
Copy link

netlify bot commented Aug 28, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 6658156

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/612a5c6660cff6000761176e

😎 Browse the preview: https://deploy-preview-9556--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Aug 28, 2021

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: 6658156

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/612a5c66931c48000750c71b

😎 Browse the preview: https://deploy-preview-9556--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented Aug 28, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: debe2a3

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/61607697b5f1850008922661

😎 Browse the preview: https://deploy-preview-9556--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Aug 28, 2021

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: ee0cd5c

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/612a5c94d24b270008db3efe

😎 Browse the preview: https://deploy-preview-9556--carbon-components-react.netlify.app

@TuxedoFish TuxedoFish marked this pull request as ready for review August 28, 2021 16:48
@TuxedoFish TuxedoFish requested a review from a team as a code owner August 28, 2021 16:48
@netlify
Copy link

netlify bot commented Aug 28, 2021

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: debe2a3

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/61607697d39e8700081607d0

😎 Browse the preview: https://deploy-preview-9556--carbon-components-react.netlify.app

@tw15egan
Copy link
Collaborator

This is really great! It's looking good when aligned to the left of the screen, but seeing some issues when I set justifyContent: flex-end and align to the right of the screen

Screen Shot 2021-08-30 at 1 11 33 PM

Screen Shot 2021-08-30 at 1 11 10 PM

@TuxedoFish
Copy link
Contributor Author

TuxedoFish commented Aug 30, 2021

This is really great! It's looking good when aligned to the left of the screen, but seeing some issues when I set justifyContent: flex-end and align to the right of the screen

Screen Shot 2021-08-30 at 1 11 33 PM Screen Shot 2021-08-30 at 1 11 10 PM

Thanks so much 😄 !

Ahh yes so I saw this but then I found I could recreate this on the public storybook for 7.42.0 (https://react.carbondesignsystem.com/?path=/story/components-tooltip--default-bottom)

bottom-issue
top-issue

All this PR should do in theory is if tooltip direction is bottom and that would move it outside of the container, then it switches it to top. But then in this case it looks like the edge case of positioning it up against the right edge seems to be not quite behaving as expected?

I can have a look into why that is happenning but to me, it seems to be an existing issue not something added by this change, do you want me to work on fixing it in this PR @tw15egan?

@tw15egan
Copy link
Collaborator

Right you are @TuxedoFish, I'm also seeing the behavior in the existing storybook. I'll create a separate issue, and if you'd like, you can submit a separate PR from this one. Since this is an existing bug, and your PR introduces new functionality, I'd like to keep them separate. Thanks for digging into the issue! 😄

@TuxedoFish
Copy link
Contributor Author

Right you are @TuxedoFish, I'm also seeing the behavior in the existing storybook. I'll create a separate issue, and if you'd like, you can submit a separate PR from this one. Since this is an existing bug, and your PR introduces new functionality, I'd like to keep them separate. Thanks for digging into the issue! 😄

Awesome, yeh I would be happy to work on that!

Copy link
Collaborator

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for contributing!

@TuxedoFish
Copy link
Contributor Author

LGTM, thanks for contributing!

Thanks!

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

@TuxedoFish Thanks for taking a look at this!

Adding to the flex-end situation a bit - I wonder if the initial positioning logic is the cause? I can replicate the placement being incorrect, but as soon as the browser window is resized, it snaps into place:

flex-end.with.resize.mov

I also see similar incorrect behavior with the default flex/justify properties on small viewport sizes with resize:

small.sizes.and.resize.mov

@tay1orjones
Copy link
Member

tay1orjones commented Aug 31, 2021

@TuxedoFish I also think it could be helpful for the auto orientation story to render four tooltips/triggers instead of just one. If one was placed in each corner of the screen, it would be very easy to see how the auto orientation prop augments the positioning logic to avoid placing tooltips outside of the container.

@TuxedoFish
Copy link
Contributor Author

@TuxedoFish Thanks for taking a look at this!

Adding to the flex-end situation a bit - I wonder if the initial positioning logic is the cause? I can replicate the placement being incorrect, but as soon as the browser window is resized, it snaps into place:

flex-end.with.resize.mov
I also see similar incorrect behavior with the default flex/justify properties on small viewport sizes with resize:

small.sizes.and.resize.mov

Yes I see that too, I also noticed that if I output the bounding rectangle that is used in Floating Menu before and after resizing the window, it is getting different widths and heights.

glitch.mp4

I saw that the bx--tooltip has a min-width and max-width so the width can be a certain range. It seems like when the tooltip is positioned for the first time on those edges it then seems to assume a larger size which then means the position looks off potentially? Not sure how to go about fixing this?

I have also pushed that change, sounds like a good idea, I have kept them all using the same props from storybook and then positioned them in the four corners!

@tay1orjones
Copy link
Member

@TuxedoFish if it's a problem of having outdated/invalid parameters of the positioning values, I wonder if moving the logic to getSnapshotBeforeUpdate instead of componentDidUpdate would be useful 🤔

@TuxedoFish
Copy link
Contributor Author

@TuxedoFish if it's a problem of having outdated/invalid parameters of the positioning values, I wonder if moving the logic to getSnapshotBeforeUpdate instead of componentDidUpdate would be useful 🤔

I gave it a try and it didn't solve it straight away but I'll have to investigate further! @tay1orjones are you happy for me to work on this in a separate PR for #9561.

If so is there any changes you want me to make to this PR or are you happy to approve? Thanks,

@TuxedoFish
Copy link
Contributor Author

I have opened #9606 as a potential fix for the alignment of tooltips on the right hand side (#9561)

@Harry-Liversedge
Copy link
Contributor

Harry-Liversedge commented Oct 6, 2021

Hey @tay1orjones, hope you're doing well! Appreciate you will be super busy, but would you be able to take a look at this PR when you get a chance? (Sorry meant to post as "TuxedoFish")

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

Sorry for the delay @TuxedoFish - this looks great, thanks for the contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tooltip is escaping the screen when it is close right left bottom f the screen
4 participants