fix(action-menu): add a slot for Tooltip content #3488

anishAdobe · 2023-07-25T10:05:21Z

Tooltip for ActionMenu

Description

Related issue(s)

#2916

Motivation and context

How has this been tested?

Test case 1
1. Go here
2. Do this
Test case 2
1. Go here
2. Do this

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

I have signed the Adobe Open Source CLA.
My code follows the code style of this project.
If my change required a change to the documentation, I have updated the documentation in this pull request.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.
I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Best practices

This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.

github-actions · 2023-07-25T10:09:04Z

Branch Preview

Visual regression test results

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 · 2023-07-25T10:15:03Z

Tachometer results

Chrome

action-menu permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	748 kB	242.21ms - 248.66ms	-	unsure 🔍 -2% - +2% -5.08ms - +4.17ms
branch	740 kB	242.58ms - 249.20ms	unsure 🔍 -2% - +2% -4.17ms - +5.08ms	-

Firefox

action-menu permalink

Version	Bytes	Avg Time	vs remote	vs branch
npm latest	748 kB	408.39ms - 441.17ms	-	unsure 🔍 -3% - +6% -12.98ms - +26.82ms
branch	740 kB	406.57ms - 429.15ms	unsure 🔍 -6% - +3% -26.82ms - +12.98ms	-

Westbrook

I need to discuss with the team available APIs here for uniformity across the project, but if this version is the right one then you'll be on a good path for this merge with these notes.

Westbrook · 2023-07-26T23:48:16Z

packages/action-menu/stories/index.ts

@@ -52,6 +54,9 @@ export const ActionMenuMarkup = ({
            <sp-menu-divider></sp-menu-divider>
            <sp-menu-item>Save Selection</sp-menu-item>
            <sp-menu-item disabled>Make Work Path</sp-menu-item>
+            ${tooltip_description
+                ? html`<sp-tooltip slot='tooltip' self-managed placement=${tooltip_placement}>${tooltip_description}</sp-tooltip`


There's no definition for <sp-tooltip> included in this file.

There's also a missing >.

Westbrook · 2023-07-26T23:49:04Z

packages/action-menu/stories/action-menu.stories.ts

@@ -114,6 +159,14 @@ iconOnly.args = {
    visibleLabel: '',
 };

+export const tooltip_description_placement = (


Convert these snake_case variables to camelCase to match the rest of the project.

packages/action-menu/src/ActionMenu.ts

Westbrook

Baring the notes in this review, we think this approach is the correct path forward.

Concerning the issues you experienced with Overlay V2, we see that as a feature that should be added to <sp-tooltip> in that context, and doesn't need to derail the work you are doing here.

packages/action-menu/README.md

Westbrook · 2023-08-01T14:45:57Z

packages/action-menu/README.md

+
+```html
+<sp-action-menu
+    <sp-tooltip slot='tooltip' self-managed placement='bottom'>'Content'</sp-tooltip>`


Suggested change

<sp-tooltip slot='tooltip' self-managed placement='bottom'>'Content'</sp-tooltip>`

<sp-tooltip slot='tooltip' self-managed placement='bottom'>'Content'</sp-tooltip>

Westbrook · 2023-08-01T14:46:13Z

packages/action-menu/README.md

+Tooltip in action menu can be added via a slot. It can be attached to action menu by adding <sp-action-menu> and giving various parameters (e.g. placement, content, etc) as needed.
+
+```html
+<sp-action-menu


Suggested change

<sp-action-menu

<sp-action-menu>

Westbrook · 2023-08-01T14:56:22Z

packages/action-menu/stories/action-menu.stories.ts

@@ -114,6 +159,14 @@ iconOnly.args = {
    visibleLabel: '',
 };

+export const tooltipDescriptionAndPlacement = (


Please ship this story without a visible label.

Westbrook · 2023-08-01T15:44:09Z

packages/action-menu/README.md

+
+## Adding tootip in action menu
+
+Tooltip in action menu can be added via a slot. It can be attached to action menu by adding <sp-action-menu> and giving various parameters (e.g. placement, content, etc) as needed.


This doesn't seem right.

Westbrook · 2023-08-01T16:38:27Z

packages/action-menu/src/ActionMenu.ts

@@ -55,6 +56,7 @@ export class ActionMenu extends ObserveSlotText(PickerBase, 'label') {
                    <sp-icon-more class="icon"></sp-icon-more>
                </slot>
                <slot name="label" ?hidden=${!this.hasLabel}></slot>
+                <slot name="tooltip" ?hidden=${this.open}></slot>


I do not believe this is necessary. As noted, we'll address the Tooltip functionality in Overlay V2 as part of the Tooltip in that work.

This is temporary fix. Once this issue will be addressed in Tooltip functionality in Overlay V2. Will remove it.

This is not required in the V1 API and leaving it "fixed" will make it harder to include the actual fix in the V2 PR. Please remove.

Westbrook

The code here looks great!

Can you add back the story that was here a couple of commits back? My comment on it was to make sure that it didn't have a visible label by default, not to remove it. Sorry if I was unclear.

With that back in (and the related VRT updates), we should be good to ship this. Thanks!

packages/action-menu/stories/action-menu.stories.ts

.circleci/config.yml

Westbrook

Thanks for working through this with us! Looks good to go 🥳

anishAdobe marked this pull request as ready for review July 26, 2023 06:23

anishAdobe changed the title ~~Users/anishr/actionmenu~~ Feat: Tooltip for Action Menu Jul 26, 2023

anishAdobe requested review from Westbrook and vjosyula July 26, 2023 06:26

anishAdobe force-pushed the users/anishr/actionmenu branch from 64fd49b to e7aa92d Compare July 26, 2023 12:16

anishAdobe changed the title ~~Feat: Tooltip for Action Menu~~ feat(action-menu): Tooltip for Action Menu Jul 26, 2023

anishAdobe force-pushed the users/anishr/actionmenu branch from 55a6a17 to 004e735 Compare July 26, 2023 12:21

Westbrook reviewed Jul 26, 2023

View reviewed changes

anishAdobe requested a review from Westbrook July 27, 2023 05:57

anishAdobe force-pushed the users/anishr/actionmenu branch 2 times, most recently from b4d01a0 to f92d3dc Compare August 1, 2023 09:58

Westbrook reviewed Aug 1, 2023

View reviewed changes

anishAdobe force-pushed the users/anishr/actionmenu branch from eb1486c to bfba140 Compare August 1, 2023 16:22

Westbrook reviewed Aug 1, 2023

View reviewed changes

anishAdobe force-pushed the users/anishr/actionmenu branch from f3f87d1 to e4dc019 Compare August 2, 2023 05:07

anishAdobe requested a review from Westbrook August 2, 2023 05:08

Anish Ranjan added 10 commits August 2, 2023 11:14

feat: action menu tooltip

5b05412

feat: storybook changes

46d9dcc

feat(action-menu): tooltip slot

4a5e505

feat(action-menu): removed extra import

e88b4e2

feat(action-menu): removed extra import

76cd9d3

feat(action-menu): fixed comments

764d762

feat(action-menu): added slot description

f62d23e

feat(action-menu): space fixed

2adbfb2

chore(action-menu): hide slot when not needed

083042e

chore(action-menu): update comments

dfa44f5

anishAdobe force-pushed the users/anishr/actionmenu branch from 971a6f9 to dfa44f5 Compare August 2, 2023 05:45

chore(action-menu): update hash

70a29ef

chore(action-menu): added hide slot

5e2cb1d

anishAdobe force-pushed the users/anishr/actionmenu branch from d4684d9 to 5e2cb1d Compare August 2, 2023 07:48

Anish Ranjan added 2 commits August 2, 2023 13:37

chore(action-menu): comment added

c7be21d

chore(action-menu): removed hidden in slot

7519361

Westbrook reviewed Aug 2, 2023

View reviewed changes

Anish Ranjan added 2 commits August 2, 2023 19:34

chore(action-menu): remove visible label

0daed21

chore(action-menu): update golden hash

eabefcf

Westbrook reviewed Aug 2, 2023

View reviewed changes

packages/action-menu/stories/action-menu.stories.ts Outdated Show resolved Hide resolved

anishAdobe requested a review from Westbrook August 2, 2023 14:43

chore: accept code review

0ed74a6

Westbrook reviewed Aug 2, 2023

View reviewed changes

.circleci/config.yml Outdated Show resolved Hide resolved

ci: update golden images hash

cd53a8b

Westbrook reviewed Aug 2, 2023

View reviewed changes

.circleci/config.yml Outdated Show resolved Hide resolved

ci: update golden images hash

5b1f77a

anishAdobe requested a review from Westbrook August 2, 2023 15:34

Westbrook changed the title ~~feat(action-menu): Tooltip for Action Menu~~ fix(action-menu): add a slot for Tooltip content Aug 2, 2023

Westbrook approved these changes Aug 2, 2023

View reviewed changes

Westbrook merged commit 23cef3a into main Aug 2, 2023

Westbrook deleted the users/anishr/actionmenu branch August 2, 2023 16:14

Westbrook mentioned this pull request Aug 21, 2023

[Feat]: tooltip for action-menu #2916

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(action-menu): add a slot for Tooltip content #3488

fix(action-menu): add a slot for Tooltip content #3488

anishAdobe commented Jul 25, 2023

github-actions bot commented Jul 25, 2023

github-actions bot commented Jul 25, 2023 •

edited

Loading

action-menu permalink

action-menu permalink

Westbrook left a comment

Westbrook Jul 26, 2023

Westbrook Jul 26, 2023

Westbrook left a comment

Westbrook Aug 1, 2023

Westbrook Aug 1, 2023

Westbrook Aug 1, 2023

Westbrook Aug 1, 2023

Westbrook Aug 1, 2023

anishAdobe Aug 2, 2023 •

edited

Loading

Westbrook Aug 2, 2023

Westbrook left a comment

Westbrook left a comment

	<sp-tooltip slot='tooltip' self-managed placement='bottom'>'Content'</sp-tooltip>`
	<sp-tooltip slot='tooltip' self-managed placement='bottom'>'Content'</sp-tooltip>


		## Adding tootip in action menu

		Tooltip in action menu can be added via a slot. It can be attached to action menu by adding <sp-action-menu> and giving various parameters (e.g. placement, content, etc) as needed.

fix(action-menu): add a slot for Tooltip content #3488

fix(action-menu): add a slot for Tooltip content #3488

Conversation

anishAdobe commented Jul 25, 2023

Description

Related issue(s)

Motivation and context

How has this been tested?

Screenshots (if appropriate)

Types of changes

Checklist

Best practices

github-actions bot commented Jul 25, 2023

Branch Preview

github-actions bot commented Jul 25, 2023 • edited Loading

Tachometer results

action-menu permalink

action-menu permalink

Westbrook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Westbrook left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anishAdobe Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Westbrook left a comment

Choose a reason for hiding this comment

Westbrook left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 25, 2023 •

edited

Loading

anishAdobe Aug 2, 2023 •

edited

Loading