fix(tooltip): rtl places tip in correct position #2736

mdt2 · 2024-05-06T16:27:00Z

Description

This PR aims to address issue #2707 by adjusting the left and right positions of the Tooltip for RTL. Also included is an updated Chromatic kitchen sink story to increase VRT coverage.

How and where has this been tested?

Please tag yourself on the tests you've marked complete to confirm the tests have been run by someone other than the author.

Validation steps

Head over to Storybook and turn on the testing preview for Tooltip. You should see all the options for tooltip placement there
When you switch to RTL, all the tip placements should be touching the body of the tooltip
When you switch to RTL, the left and right tooltips are facing the correct way (which I believe should be opposite in RTL, so if the heading says "left" the tooltip should be to the right of the imaginary source when RTL is on)
Check the docs site in LTR and RTL. Confirm no regressions, specifically for "show on hover" section

Regression testing

Validate:

The documentation pages for at least two other components are still loading, including:

The pages render correctly, are accessible, and are responsive.

If components have been modified, VRTs have been run on this branch:

VRTs have been run and looked at.
Any VRT changes have been accepted (by reviewer and/or PR author), or there are no changes.

Screenshots

To-do list

I have read the contribution guidelines.
I have updated relevant storybook stories and templates.
I have tested these changes in Windows High Contrast mode.
If my change impacts other components, I have tested to make sure they don't break.
If my change impacts documentation, I have updated the documentation accordingly.
✨ This pull request is ready to merge. ✨

changeset-bot · 2024-05-06T16:27:03Z

🦋 Changeset detected

Latest commit: 21d4cf4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@spectrum-css/tooltip	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2024-05-06T16:29:50Z

🚀 Deployed on https://pr-2736--spectrum-css.netlify.app

github-actions · 2024-05-06T16:35:40Z

File metrics

Summary

Total size: 4.48 MB^*
Total change (Δ): ⬆ 2.72 KB (0.06%)

Table reports on changes to a package's main file. Other changes can be found in the collapsed Details section below.

Package	Size	Δ
tooltip	34.03 KB	⬆ 0.68 KB

Details

tooltip

File	Head	Base	Δ
index-base.css	33.77 KB	33.11 KB	⬆ 0.68 KB (1.99%)
index-theme.css	0.86 KB	0.86 KB	-
index-vars.css	34.03 KB	33.37 KB	⬆ 0.68 KB (1.98%)
index.css	34.03 KB	33.37 KB	⬆ 0.68 KB (1.98%)
metadata.json	24.04 KB	23.29 KB	⬆ 0.76 KB (3.20%)
themes/express.css	0.73 KB	0.73 KB	-
themes/spectrum.css	0.73 KB	0.73 KB	-

* Size determined by adding together the size of the main file for all packages in the library.
* Results are not gzipped or minified.
* An ASCII character in UTF-8 is 8 bits or 1 byte.

rise-erpelding

This looks great! I'm approving because this fixes the bug and the Chromatic coverage is a big improvement! 🎉

I have a question about something that existed before and may not necessarily need to be fixed:
I find these Bottom Left/Bottom Right/Top Left/Top Right tooltips very confusing when they're presented in rtl (see screenshots below). It does make sense if they're supposed to flip directions in rtl so that they're not actually left or right, but thought it would be good to confirm that's how we want them while we're here.

mdt2 · 2024-05-07T13:50:16Z

Great question @rise-erpelding! When RTL is on, the assumption is that the component would show a right-to-left language like Arabic, so the flipping of the UI aligns with that. I agree that calling it Left/Right is confusing in this case. We also have Start/End that look the same as Left/Right. I'm wondering if the intention in adding those was to eventually deprecate Left/Right in favor of the more Logical Property aligned naming. If that's the case, this would be a great thing to deprecate as part of the S2 migration. I'll look into doing that.

jawinn

This looks good and fixes the issue with the tip placement.

I did notice one thing that seems to have regressed, that may be a slightly different bug. It might be worth trying to tackle here as well? Or a followup PR/issue.

In the YML docs, under "Show on hover", these don't appear to behave the same way anymore in RTL. These are using the CSS within a .u-tooltip-showOnHover class (which seems unusual for our library):

Older version built on prod:

Current local build of main and PR:

jawinn · 2024-05-06T19:46:10Z

components/tooltip/stories/tooltip.stories.js

+const PlacementVariants = (args) => html`
+	${window.isChromatic()
+		? html`
+			${placementOptions.map(option => {


👍 Nice addition that displays all the placement options

mdt2 · 2024-05-07T15:17:28Z

@jawinn I can't seem to replicate this, maybe I'm misunderstanding 🤔 Are you looking at "Show on hover" for the PR deploy of this PR? This is what I see:

jawinn · 2024-05-07T15:38:34Z

I can't seem to replicate this, maybe I'm misunderstanding 🤔 Are you looking at "Show on hover" for the PR deploy of this PR? This is what I see:

Yes, on that PR link. Viewed in Direction RTL.

mdt2 · 2024-05-08T12:36:45Z

components/tooltip/index.css

+	/* stylelint-disable-next-line csstools/use-logical -- intentional use of non-logical property */
+	left: 50%;


I found that the issue @jawinn was seeing for RTL text direction in the "Show on hover" section of the docs site was caused by a regression we missed in a recent PR. I waffled on what would be the best fix for this, debating between the option to use non-logical properties or to switch the logical property in a dir(rtl) selector. I landed on using non-logical properties intentionally because of the specifications about how the placements are intended to work, which I've documented in the tooltip.yml.

jawinn

Looks great! Thanks for fixing that "Show on hover" issue and for adding documentation.

mdt2 force-pushed the mdt2/css-749-tooltip-rtl branch from e6d28af to 46a6ec9 Compare May 6, 2024 16:29

mdt2 added the run_vrt For use on PRs looking to kick off VRT label May 6, 2024

mdt2 removed the run_vrt For use on PRs looking to kick off VRT label May 6, 2024

mdt2 requested review from pfulton, jawinn, castastrophe and rise-erpelding May 6, 2024 16:40

mdt2 added the ready-for-review label May 6, 2024

rise-erpelding approved these changes May 7, 2024

View reviewed changes

jawinn approved these changes May 7, 2024

View reviewed changes

mdt2 force-pushed the mdt2/css-749-tooltip-rtl branch from 46a6ec9 to 1490073 Compare May 7, 2024 16:07

mdt2 commented May 8, 2024

View reviewed changes

mdt2 force-pushed the mdt2/css-749-tooltip-rtl branch from 2c48cf6 to 774d2d0 Compare May 8, 2024 12:38

mdt2 requested a review from jawinn May 8, 2024 12:40

jawinn approved these changes May 8, 2024

View reviewed changes

mdt2 added 3 commits May 8, 2024 10:47

fix(tooltip): rtl places tip in correct position

814ea46

fix: alignment of tip on hover for rtl top/bottom

f0821d1

fix: alignment of tooltip to source on hover

21d4cf4

mdt2 force-pushed the mdt2/css-749-tooltip-rtl branch from 774d2d0 to 21d4cf4 Compare May 8, 2024 14:48

pfulton added the run_vrt For use on PRs looking to kick off VRT label May 8, 2024

pfulton linked an issue May 8, 2024 that may be closed by this pull request

[Tooltip] Left/right tooltips in RTL have incorrect tip placements #2707

Closed

pfulton merged commit 68117da into main May 8, 2024
22 checks passed

pfulton deleted the mdt2/css-749-tooltip-rtl branch May 8, 2024 18:14

github-actions bot mentioned this pull request May 8, 2024

chore: release #2738

Merged

This was referenced May 9, 2024

feat(tooltip): S2 migration #2743

Open

fix(popover): correct left/right tips for RTL, increase VRT coverage #2753

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(tooltip): rtl places tip in correct position #2736

fix(tooltip): rtl places tip in correct position #2736

mdt2 commented May 6, 2024 •

edited by jawinn

changeset-bot bot commented May 6, 2024 •

edited

github-actions bot commented May 6, 2024 •

edited

github-actions bot commented May 6, 2024 •

edited

tooltip

rise-erpelding left a comment

mdt2 commented May 7, 2024 •

edited

jawinn left a comment

jawinn May 6, 2024

mdt2 commented May 7, 2024

jawinn commented May 7, 2024

mdt2 May 8, 2024 •

edited

jawinn left a comment

		/* stylelint-disable-next-line csstools/use-logical -- intentional use of non-logical property */
		left: 50%;

fix(tooltip): rtl places tip in correct position #2736

fix(tooltip): rtl places tip in correct position #2736

Conversation

mdt2 commented May 6, 2024 • edited by jawinn

Description

How and where has this been tested?

Validation steps

Regression testing

Screenshots

To-do list

changeset-bot bot commented May 6, 2024 • edited

🦋 Changeset detected

github-actions bot commented May 6, 2024 • edited

github-actions bot commented May 6, 2024 • edited

File metrics

Summary

tooltip

rise-erpelding left a comment

Choose a reason for hiding this comment

mdt2 commented May 7, 2024 • edited

jawinn left a comment

Choose a reason for hiding this comment

jawinn May 6, 2024

Choose a reason for hiding this comment

mdt2 commented May 7, 2024

jawinn commented May 7, 2024

mdt2 May 8, 2024 • edited

Choose a reason for hiding this comment

jawinn left a comment

Choose a reason for hiding this comment

mdt2 commented May 6, 2024 •

edited by jawinn

changeset-bot bot commented May 6, 2024 •

edited

github-actions bot commented May 6, 2024 •

edited

github-actions bot commented May 6, 2024 •

edited

mdt2 commented May 7, 2024 •

edited

mdt2 May 8, 2024 •

edited