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,tourtip,infotip): added popper support #2165

Merged
merged 13 commits into from
Dec 22, 2023
Merged

Conversation

agliga
Copy link
Contributor

@agliga agliga commented Oct 2, 2023

Fixes #1424

  • This PR contains CSS changes
  • This PR does not contain CSS changes

Description

  • Added support for popperjs. Changed all out examples to use popper
  • I removed the use of makeup expander, since all it seems like it was doing was to toggle aria-expanded. We can revisit this if needed, but it doesn't seem necessary.
  • I did not touch any storybooks since they are static. The old code should work as if if the users opt not to use popper (so there is no breaking changes)

Checklist

  • I verify the build is in a non-broken state
  • I verify all changes are within scope of the linked issue
  • I regenerated all CSS files under dist folder
  • I tested the UI in all supported browsers
  • I did a visual regression check of the components impacted by doing a Percy build and approved the build
  • I tested the UI in dark mode and RTL mode
  • I added/updated/removed Storybook coverage as appropriate

@agliga agliga self-assigned this Oct 2, 2023
@ianmcburnie
Copy link
Contributor

ianmcburnie commented Oct 2, 2023

Just noticed this on the popper documentation:

Safari has unfortunate quirks regarding updating the position of DOM elements. We have filed WebKit bugs for these, so hopefully they will be fixed in the future.

Might be worth looking at those issues to make sure we are comfortable with them. We have to support back to Safari 12.

@ArtBlue
Copy link
Contributor

ArtBlue commented Oct 3, 2023

Just noticed this on the popper documentation:

Safari has unfortunate quirks regarding updating the position of DOM elements. We have filed WebKit bugs for these, so hopefully they will be fixed in the future.

Might be worth looking at those issues to make sure we are comfortable with them. We have to support back to Safari 12.

Those issues look to be related to scrolling, one of them over-scrolling (rubber band) effect. I can see why they would occur. If the intent of the library is to attach one element to another positionally and also to avoid viewport boundaries and reposition itself within the viewport, I can see how scrolling would create a "conflict of interest" here.

docs/_includes/infotip.html Outdated Show resolved Hide resolved
@agliga
Copy link
Contributor Author

agliga commented Oct 3, 2023

Just noticed this on the popper documentation:

Safari has unfortunate quirks regarding updating the position of DOM elements. We have filed WebKit bugs for these, so hopefully they will be fixed in the future.

Might be worth looking at those issues to make sure we are comfortable with them. We have to support back to Safari 12.

Those issues look to be related to scrolling, one of them over-scrolling (rubber band) effect. I can see why they would occur. If the intent of the library is to attach one element to another positionally and also to avoid viewport boundaries and reposition itself within the viewport, I can see how scrolling would create a "conflict of interest" here.

Yah from what i see that on safari it would act just like what we had before. So it wont be any different from our previous implementation of tip overlays.

ArtBlue
ArtBlue previously approved these changes Oct 3, 2023
Copy link
Contributor

@ArtBlue ArtBlue left a comment

Choose a reason for hiding this comment

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

LGTM.

@ianmcburnie
Copy link
Contributor

ianmcburnie commented Oct 3, 2023

Is[data-popper-placement] added with JavaScript? Or is it declared in the base markup? I'm guessing the former as I don't see the attribute displayed in the example code. Which leads me to wonder whether we should be approaching this in a progressive enhancement fashion. Our existing modifiers and CSS work for most simple cases, so I don't think we need to always throw that away. It gives us a pretty good sensible default without the need for JS. Perhaps instead we should be progressively enhancing our existing modifiers (i.e. switching out tooltip__pointer tooltip__pointer--left for the popper equivalent). Let's chat offline about this.

@ianmcburnie
Copy link
Contributor

ianmcburnie commented Oct 3, 2023

Can we please add a test story for popper overlay inside of container with overflow hidden. Or even showcase that in the main docs.

@ianmcburnie
Copy link
Contributor

Not seeing the overlay or pointer appear in this example in Safari:

Screenshot 2023-10-03 at 12 56 35 PM

Here it is in Chrome:

Screenshot 2023-10-03 at 12 57 14 PM

@agliga
Copy link
Contributor Author

agliga commented Oct 13, 2023

Will wait for next release to get all the cleanup.

@agliga agliga changed the base branch from 16.8.0 to 16.9.0 October 25, 2023 15:07
Base automatically changed from 16.9.0 to master November 21, 2023 22:31
@LuLaValva LuLaValva dismissed ArtBlue’s stale review November 21, 2023 22:31

The base branch was changed.

@agliga agliga changed the base branch from master to 17.0.0 December 8, 2023 17:48
@agliga agliga marked this pull request as ready for review December 8, 2023 19:04
@agliga
Copy link
Contributor Author

agliga commented Dec 8, 2023

Okay, should be ready for re-review. Updated to floating ui, should be working.

Copy link
Contributor

@ArtBlue ArtBlue left a comment

Choose a reason for hiding this comment

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

Looks good!

@ianmcburnie
Copy link
Contributor

On tourtips, the pointers look broken (in safari & chrome):
Screenshot 2023-12-18 at 11 26 42 AM
Screenshot 2023-12-18 at 11 26 36 AM

@ianmcburnie
Copy link
Contributor

ianmcburnie commented Dec 18, 2023

Isn't this Infotip pointer supposed to flip from bottom to top when the info top repositions itself?

Screenshot 2023-12-18 at 11 29 25 AM Screenshot 2023-12-18 at 11 29 36 AM

@agliga
Copy link
Contributor Author

agliga commented Dec 18, 2023

Isn't this Infotip pointer supposed to flip from bottom to top when the info top repositions itself?

Screenshot 2023-12-18 at 11 29 25 AM Screenshot 2023-12-18 at 11 29 36 AM

Fixed, this was due to doing !== instead of != as the docs recommended.

@agliga
Copy link
Contributor Author

agliga commented Dec 18, 2023

On tourtips, the pointers look broken (in safari & chrome): Screenshot 2023-12-18 at 11 26 42 AM Screenshot 2023-12-18 at 11 26 36 AM

Added a 20px padding on arrow (The padding on the arrow is based on the border radius values so it doesn't look like this).

Copy link
Member

@LuLaValva LuLaValva left a comment

Choose a reason for hiding this comment

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

Looks awesome! I'm excited to see this on other floating elements like listbox and date-textbox.

It would have probably been nice to do prettier for main.js in a separate PR but I'll let it slide

Copy link
Contributor

@ianmcburnie ianmcburnie left a comment

Choose a reason for hiding this comment

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

I would like to still keep the examples of infotip, tooltip, tourtip that we had without popper, and then added a dynamic positioning section for each which shows what accommodations we have to make in the markup/css (if any) for a 3rd party module like popper.

@ianmcburnie
Copy link
Contributor

ianmcburnie commented Dec 20, 2023

Noticing a couple of discrepancies between positioning on prod storybook vs branch in tooltip stories:

tooltip -> expanded

Screenshot 2023-12-20 at 3 14 00 PM Screenshot 2023-12-20 at 3 13 49 PM

Tooltip -> pointer -> left (and all others)

Screenshot 2023-12-20 at 3 15 30 PM Screenshot 2023-12-20 at 3 15 27 PM

docs/_includes/infotip.html Outdated Show resolved Hide resolved
@agliga
Copy link
Contributor Author

agliga commented Dec 21, 2023

Noticing a couple of discrepancies between positioning on prod storybook vs branch in tooltip stories:

tooltip -> expanded

Screenshot 2023-12-20 at 3 14 00 PM Screenshot 2023-12-20 at 3 13 49 PM
Tooltip -> pointer -> left (and all others)

Screenshot 2023-12-20 at 3 15 30 PM Screenshot 2023-12-20 at 3 15 27 PM

Fixed this. I had removed the margin on bubble which was breaking in other places. I changed the positioning of the tip to get it to work.

Copy link
Contributor

@ArtBlue ArtBlue left a comment

Choose a reason for hiding this comment

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

Looks fine to me, but we should get a confirmation approval from Ian since he had the bulk of feedback.

Copy link
Contributor

@ianmcburnie ianmcburnie left a comment

Choose a reason for hiding this comment

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

Great work @agliga

@agliga agliga merged commit ff81aeb into 17.0.0 Dec 22, 2023
2 checks passed
@agliga agliga deleted the popper-tips branch December 22, 2023 22:03
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.

4 participants