refactor: update tooltip usage #1673

Westbrook · 2021-07-31T19:10:12Z

Description

Investigate surfacing tooltips via an alternate API.

Related Issue

refs #1436

Motivation and Context

Less DOM is better DOM.

How Has This Been Tested?

needs tests
needs VRTs

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

github-actions · 2021-07-31T19:13:27Z

Branch Preview

When a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:

github-actions · 2021-07-31T19:25:08Z

Tachometer results

action-bar permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	314 kB	26.77ms - 27.68ms	-	unsure 🔍 -2% - +2% -0.50ms - +0.58ms
branch	312 kB	26.88ms - 27.48ms	unsure 🔍 -2% - +2% -0.58ms - +0.50ms	-

action-menu permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	790 kB	333.58ms - 339.48ms	-	unsure 🔍 -0% - +2% -1.37ms - +6.81ms
branch	784 kB	330.97ms - 336.64ms	unsure 🔍 -2% - +0% -6.81ms - +1.37ms	-

overlay permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	416 kB	103.02ms - 105.49ms	-	unsure 🔍 -2% - +1% -1.73ms - +1.48ms
branch	414 kB	103.35ms - 105.40ms	unsure 🔍 -1% - +2% -1.48ms - +1.73ms	-

picker permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	657 kB	868.32ms - 884.14ms	-	unsure 🔍 -1% - +1% -8.50ms - +12.59ms
branch	652 kB	867.21ms - 881.16ms	unsure 🔍 -1% - +1% -12.59ms - +8.50ms	-

popover permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	317 kB	42.80ms - 43.60ms	-	faster ✔ 1% - 4% 0.23ms - 1.77ms
branch	315 kB	43.54ms - 44.86ms	slower ❌ 1% - 4% 0.23ms - 1.77ms	-

split-button permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	730 kB	2654.19ms - 2690.12ms	-	unsure 🔍 -0% - +2% -9.32ms - +46.44ms
branch	725 kB	2632.28ms - 2674.92ms	unsure 🔍 -2% - +0% -46.44ms - +9.32ms	-

tooltip permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	320 kB	38.05ms - 39.14ms	-	faster ✔ 2% - 6% 0.91ms - 2.33ms
branch	321 kB	39.76ms - 40.68ms	slower ❌ 2% - 6% 0.91ms - 2.33ms	-

Westbrook · 2021-10-21T14:24:48Z

packages/tooltip/src/Tooltip.ts

+
+    private openOverlay = async (): Promise<void> => {
+        const parentElement = this.parentElement as HTMLElement;
+        this.closeOverlayCallback = await openOverlay(


Just realizing that this PR exhibits the same asynchronous issue that we recently massages out of overlay-trigger 😱 I'll clean this up today so that it matches what we ended up with in that element, which is much nicer!

This might also have something to do with why overlay-trigger tooltips and sp-tooltip[auto] surface content to the screen reader differently. Will review that more closely as well!

packages/tooltip/src/Tooltip.ts

hunterloftis

Sorry to bikeshed, but is auto the best description for this inverted tooltip API for a user who isn't necessarily expert at the underlying implementations? I wonder if something like nested, contained, or inner might be more intuitive.

packages/overlay/src/ActiveOverlay.ts

hunterloftis · 2021-10-21T15:07:25Z

packages/tooltip/src/Tooltip.ts

+    private closeOverlay = (): void => {
+        if (this.closeOverlayCallback) {
+            this.closeOverlayCallback();
+            delete this.closeOverlayCallback;
+        }
+    };


We might exit early here to reduce a level of nesting:

Suggested change

private closeOverlay = (): void => {

if (this.closeOverlayCallback) {

this.closeOverlayCallback();

delete this.closeOverlayCallback;

}

};

private closeOverlay(): void {

if (!this.closeOverlayCallback) return;

this.closeOverlayCallback();

delete this.closeOverlayCallback;

};

Westbrook · 2021-10-25T19:28:36Z

@hunterloftis I get your request for renaming this attribute, but I'm not sure I can agree to any of the suggested. If managed or self-managed an acceptable alternative?

In a breaking API change, I might prefer to call this default and the versions that require wrapping managed. Maybe we wait till 1.0 and do something like that? 🤔

hunterloftis · 2021-10-26T14:32:03Z

@Westbrook I can get behind managed. I guess the real information to be conveyed is that it's "automatically managed" 😁 but that would be a mouthful of an attribute! (self-managed works as well for me)

hunterloftis · 2021-10-26T14:40:51Z

@Westbrook before re-approving I was going to run through a quick manual test. Do we have a new storybook link? https://westbrook-tooltip-alt--spectrum-web-components.netlify.app/storybook/ (from the above bot) loads the docs site.

Westbrook · 2021-10-26T18:59:47Z

I went with self-managed for now, but I do think that the inverse would be preferable post 1.0. I've created #1885 to track.

Once the CI runs through we should get Storybook back 🙈

hunterloftis · 2021-10-26T21:11:07Z

I'm seeing some unexpected behavior in the storybook:

self-managed-tooltip-sizing-artifacts.mov

Westbrook · 2021-10-26T21:12:32Z

packages/tooltip/src/tooltip.css

@@ -19,6 +19,10 @@ governing permissions and limitations under the License.
    border: none;
 }

+:host([auto]:not([aria-hidden])) {


Look who didn't get updated 👀 😱

Westbrook force-pushed the westbrook/tooltip-alt branch 4 times, most recently from bd6d81f to c0c2c65 Compare August 5, 2021 19:56

Westbrook force-pushed the westbrook/tooltip-alt branch 5 times, most recently from 793d387 to 404c109 Compare August 12, 2021 01:12

Westbrook force-pushed the westbrook/tooltip-alt branch from 404c109 to ab043e9 Compare August 31, 2021 01:33

Westbrook force-pushed the westbrook/tooltip-alt branch from ab043e9 to e985fda Compare September 10, 2021 01:04

Westbrook force-pushed the westbrook/tooltip-alt branch 2 times, most recently from ed77786 to 0625924 Compare October 5, 2021 16:00

Westbrook force-pushed the westbrook/tooltip-alt branch from 0625924 to 5e64b35 Compare October 18, 2021 19:51

Westbrook marked this pull request as ready for review October 19, 2021 19:57

Westbrook force-pushed the westbrook/tooltip-alt branch 2 times, most recently from fa70b4c to 72bc047 Compare October 19, 2021 21:48

Westbrook changed the title ~~refactor: update tooltip usage, wip~~ refactor: update tooltip usage Oct 19, 2021

hunterloftis self-assigned this Oct 21, 2021

Westbrook commented Oct 21, 2021

View reviewed changes

packages/tooltip/src/Tooltip.ts Outdated Show resolved Hide resolved

hunterloftis reviewed Oct 21, 2021

View reviewed changes

Westbrook force-pushed the westbrook/tooltip-alt branch 4 times, most recently from f8a846c to c178b66 Compare October 25, 2021 19:21

Westbrook force-pushed the westbrook/tooltip-alt branch from c178b66 to 88ac612 Compare October 25, 2021 19:48

Westbrook force-pushed the westbrook/tooltip-alt branch from dfe86b4 to 16ef0ad Compare October 26, 2021 18:56

Westbrook commented Oct 26, 2021

View reviewed changes

Westbrook force-pushed the westbrook/tooltip-alt branch from 16ef0ad to 2264db8 Compare October 26, 2021 21:16

hunterloftis previously approved these changes Oct 26, 2021

View reviewed changes

Westbrook added 2 commits October 28, 2021 08:25

refactor: support self-managed tooltip elements

b9443e2

ci: update golden image cache

f92e6c9

Westbrook dismissed hunterloftis’s stale review via f92e6c9 October 28, 2021 12:25

Westbrook force-pushed the westbrook/tooltip-alt branch from 2264db8 to f92e6c9 Compare October 28, 2021 12:25

Westbrook merged commit 82e7c08 into main Oct 28, 2021

Westbrook deleted the westbrook/tooltip-alt branch October 28, 2021 12:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: update tooltip usage #1673

refactor: update tooltip usage #1673

Westbrook commented Jul 31, 2021

github-actions bot commented Jul 31, 2021 •

edited

Loading

github-actions bot commented Jul 31, 2021 •

edited

Loading

Westbrook Oct 21, 2021

Westbrook Oct 21, 2021

hunterloftis left a comment

hunterloftis Oct 21, 2021

Westbrook commented Oct 25, 2021

hunterloftis commented Oct 26, 2021 •

edited

Loading

hunterloftis commented Oct 26, 2021

Westbrook commented Oct 26, 2021

hunterloftis commented Oct 26, 2021

Westbrook Oct 26, 2021

refactor: update tooltip usage #1673

refactor: update tooltip usage #1673

Conversation

Westbrook commented Jul 31, 2021

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

github-actions bot commented Jul 31, 2021 • edited Loading

Branch Preview

github-actions bot commented Jul 31, 2021 • edited Loading

Tachometer results

action-bar permalink

action-menu permalink

overlay permalink

picker permalink

popover permalink

split-button permalink

tooltip permalink

Westbrook Oct 21, 2021

Choose a reason for hiding this comment

Westbrook Oct 21, 2021

Choose a reason for hiding this comment

hunterloftis left a comment

Choose a reason for hiding this comment

hunterloftis Oct 21, 2021

Choose a reason for hiding this comment

Westbrook commented Oct 25, 2021

hunterloftis commented Oct 26, 2021 • edited Loading

hunterloftis commented Oct 26, 2021

Westbrook commented Oct 26, 2021

hunterloftis commented Oct 26, 2021

Westbrook Oct 26, 2021

Choose a reason for hiding this comment

github-actions bot commented Jul 31, 2021 •

edited

Loading

github-actions bot commented Jul 31, 2021 •

edited

Loading

hunterloftis commented Oct 26, 2021 •

edited

Loading