chore: apply code review feedback awesomeness

Author: castastrophe

.changeset/brave-bikes-teach.md

@@ -44,10 +44,7 @@ Example markup:
 
 Updates `@spectrum-css/menu` styles to align with latest Spectrum 2 design specifications and improve accessibility.
 
-- Focus indicator tokens wired through: width, color, gap/offset, and outline style.
-- CJK line-height tokens applied for labels, descriptions, and section headers.
-- External link and drill‑in icon sizing variables exposed; thumbnail sizing and alignment refined.
-- Forced-colors improvements and readability adjustments.
+- Added this not to prevent clash with the `.is-selectable` placement.
 - Non-breaking; no class or DOM changes required.
 
 ### Action button refinements

components/actionbar/index.css

@@ -34,6 +34,7 @@
 		opacity: 1;
 	}
 
+	/* Maps to the .spectrum-Popover component */
 	.spectrum-Popover {
 		/* popover is ActionBar height */
 		block-size: var(--spectrum-action-bar-height);

components/actionbutton/dist/metadata.json

@@ -14,16 +14,16 @@
     ".spectrum-ActionButton-label",
     ".spectrum-ActionButton-label:empty",
     ".spectrum-ActionButton.is-disabled",
-    ".spectrum-ActionButton.spectrum-ActionButton--emphasized:where(.is-selected, [aria-pressed=\"true\"], [aria-expanded=\"true\"])",
+    ".spectrum-ActionButton.spectrum-ActionButton--emphasized:is(.is-selected, [aria-pressed=\"true\"], [aria-expanded=\"true\"])",
     ".spectrum-ActionButton.spectrum-ActionButton--quiet",
+    ".spectrum-ActionButton.spectrum-ActionButton--quiet:is(.is-selected, [aria-pressed=\"true\"], [aria-expanded=\"true\"])",
     ".spectrum-ActionButton.spectrum-ActionButton--quiet:is(:disabled, .is-disabled, [aria-disabled=\"true\"]):not(:where(.is-selected, [aria-pressed=\"true\"], [aria-expanded=\"true\"]))",
-    ".spectrum-ActionButton.spectrum-ActionButton--quiet:where(.is-selected, [aria-pressed=\"true\"], [aria-expanded=\"true\"])",
     ".spectrum-ActionButton.spectrum-ActionButton--staticBlack",
     ".spectrum-ActionButton.spectrum-ActionButton--staticBlack.spectrum-ActionButton--quiet",
-    ".spectrum-ActionButton.spectrum-ActionButton--staticBlack:where(.is-selected, [aria-pressed=\"true\"], [aria-expanded=\"true\"])",
+    ".spectrum-ActionButton.spectrum-ActionButton--staticBlack:is(.is-selected, [aria-pressed=\"true\"], [aria-expanded=\"true\"])",
     ".spectrum-ActionButton.spectrum-ActionButton--staticWhite",
     ".spectrum-ActionButton.spectrum-ActionButton--staticWhite.spectrum-ActionButton--quiet",
-    ".spectrum-ActionButton.spectrum-ActionButton--staticWhite:where(.is-selected, [aria-pressed=\"true\"], [aria-expanded=\"true\"])",
+    ".spectrum-ActionButton.spectrum-ActionButton--staticWhite:is(.is-selected, [aria-pressed=\"true\"], [aria-expanded=\"true\"])",
     ".spectrum-ActionButton::-moz-focus-inner",
     ".spectrum-ActionButton:active",
     ".spectrum-ActionButton:after",
@@ -35,9 +35,8 @@
     ".spectrum-ActionButton:has(.spectrum-ActionButton-icon)",
     ".spectrum-ActionButton:hover",
     ".spectrum-ActionButton:is(.is-selected, [aria-pressed=\"true\"], [aria-expanded=\"true\"])",
+    ".spectrum-ActionButton:is(:disabled, .is-disabled, [aria-disabled=\"true\"])",
     ".spectrum-ActionButton:not(:has(.spectrum-ActionButton-label))",
-    ".spectrum-ActionButton:where(.is-selected, [aria-pressed=\"true\"], [aria-expanded=\"true\"])",
-    ".spectrum-ActionButton:where(:disabled, .is-disabled, [aria-disabled=\"true\"])",
     "a.spectrum-ActionButton"
   ],
   "modifiers": [

components/actionbutton/index.css

@@ -63,7 +63,7 @@ governing permissions and limitations under the License.
 		--spectrum-actionbutton-background-color-focus: var(--spectrum-gray-200);
 		--spectrum-actionbutton-background-color-disabled: transparent;
 
-		&:where(.is-selected, [aria-pressed="true"], [aria-expanded="true"]) {
+		&:is(.is-selected, [aria-pressed="true"], [aria-expanded="true"]) {
 			--spectrum-actionbutton-background-color-disabled: var(--spectrum-disabled-background-color);
 		}
 	}
@@ -117,7 +117,7 @@ governing permissions and limitations under the License.
 	}
 
 	/* expanded is specific to action menu when the menu is open */
-	&:where(.is-selected, [aria-pressed="true"], [aria-expanded="true"]) {
+	&:is(.is-selected, [aria-pressed="true"], [aria-expanded="true"]) {
 		--mod-actionbutton-background-color-default: var(--mod-actionbutton-background-color-default-selected, var(--spectrum-neutral-background-color-selected-default));
 		--mod-actionbutton-background-color-hover: var(--mod-actionbutton-background-color-hover-selected, var(--spectrum-neutral-background-color-selected-hover));
 		--mod-actionbutton-background-color-down: var(--mod-actionbutton-background-color-down-selected, var(--spectrum-neutral-background-color-selected-down));
@@ -342,7 +342,7 @@ governing permissions and limitations under the License.
 	}
 
 	/* ideal when we want to disable the button but still allow it's content to be focused */
-	&:where(:disabled, .is-disabled, [aria-disabled="true"]) {
+	&:is(:disabled, .is-disabled, [aria-disabled="true"]) {
 		background-color: var(--highcontrast-actionbutton-background-color-disabled, var(--mod-actionbutton-background-color-disabled, var(--spectrum-actionbutton-background-color-disabled)));
 		color: var(--highcontrast-actionbutton-content-color-disabled, var(--mod-actionbutton-content-color-disabled, var(--spectrum-actionbutton-content-color-disabled)));
 	}

components/actionbutton/stories/actionbutton.stories.js

@@ -107,7 +107,7 @@ export default {
 	},
 	parameters: {
 		actions: {
-			handles: ["click .spectrum-ActionButton:not([disabled])"],
+			handles: ["click .spectrum-ActionButton:not([disabled])", "mousedown .spectrum-ActionButton:not([disabled])", "mouseup .spectrum-ActionButton:not([disabled])", "touchstart .spectrum-ActionButton:not([disabled])", "touchend .spectrum-ActionButton:not([disabled])"],
 		},
 		design: {
 			type: "figma",
@@ -198,8 +198,8 @@ Quiet.parameters = {
 
 /**
  * An action button can have a hold icon (a small corner triangle). This icon indicates that holding down the action button for a
- * short amount of time can reveal a [popover](/docs/components-popover--docs) menu, which can be used, for example, to switch
- * between related actions. Note that this popover menu is not demonstrated here—this would be handled by the implementation.
+ * short amount of time (currently the standard is 300ms) can reveal a [popover](/docs/components-popover--docs) menu, which can be used, for example, to switch
+ * between related actions. Note that this popover menu is not demonstrated here; this would be handled by the implementation.
  * Because of the way padding is calculated, the hold icon must be placed before the workflow icon in the markup.
  */
 export const HoldIcon = IconOnlyOption.bind({});

components/actionbutton/stories/template.js

@@ -69,6 +69,7 @@ export const Template = ({
 	role = "button",
 } = {}, context = {}) => {
 	const { updateArgs } = context;
+
 	return html`
 		<button
 			aria-label=${ifDefined(hideLabel ? label : undefined)}
@@ -77,31 +78,36 @@ export const Template = ({
 			aria-pressed=${ifDefined(isSelected ? "true" : undefined)}
 			aria-expanded=${ifDefined(hasPopup && hasPopup !== "false" ? isOpen ? "true" : "false" : undefined)}
 			class=${classMap({
-				[rootClass]: true,
-				[`${rootClass}--size${size?.toUpperCase()}`]:
-					typeof size !== "undefined",
-				[`${rootClass}--quiet`]: isQuiet,
-				[`${rootClass}--emphasized`]: isEmphasized,
-				[`${rootClass}--static${capitalize(staticColor)}`]:
-					typeof staticColor !== "undefined",
-				["is-disabled"]: isDisabled,
-				["is-hover"]: isHovered,
-				["is-focus-visible"]: isFocused,
-				["is-active"]: isActive,
-				...customClasses.reduce((a, c) => ({ ...a, [c]: true }), {}),
-			})}
+		[rootClass]: true,
+		[`${rootClass}--size${size?.toUpperCase()}`]:
+			typeof size !== "undefined",
+		[`${rootClass}--quiet`]: isQuiet,
+		[`${rootClass}--emphasized`]: isEmphasized,
+		[`${rootClass}--static${capitalize(staticColor)}`]:
+			typeof staticColor !== "undefined",
+		["is-disabled"]: isDisabled,
+		["is-hover"]: isHovered,
+		["is-focus-visible"]: isFocused,
+		["is-active"]: isActive,
+		...customClasses.reduce((a, c) => ({ ...a, [c]: true }), {}),
+	})}
 			id=${id}
 			data-testid=${testId ?? id}
 			popovertarget=${ifDefined(hasPopup && hasPopup !== "false" ? popupId : undefined)}
 			role=${ifDefined(role)}
 			style=${styleMap(customStyles)}
 			?disabled=${isDisabled}
-			@click=${onclick ?? function() {
-				updateArgs({
-					isSelected: !isSelected
-				});
+			@click=${function () {
+				if (isDisabled) return;
+				if (typeof onclick === "function") onclick();
+				else {
+					updateArgs({
+						isSelected: !isSelected,
+						isOpen: !isOpen,
+					});
+				}
 			}}
-			@focusin=${function() {
+			@focusin=${function () {
 				updateArgs({ isFocused: true });
 			}}
 			@focusout=${function() {

components/actionmenu/stories/actionmenu.stories.js

@@ -75,11 +75,6 @@ export default {
 	tags: ["migrated"],
 };
 
-/**
- * Action menu allows users to access and execute various commands or tasks related to their current workflow. It's typically triggered from an action button by user interaction.
- *
- * Note that the accessibility roles are different for an action menu compared to a normal menu. The action menu is a combination of a menu, popover, and action button.
- */
 export const Default = ActionMenuGroup.bind({});
 Default.args = {
 	triggerArgs: {
@@ -130,6 +125,8 @@ Default.args = {
  * By default, the menu is opened by pressing the trigger element or activating it via the <kbd>Space</kbd> or <kbd>Enter</kbd> keys. However, there may be cases where the trigger should perform a separate action on press such as selection, and should only display the menu when long pressed. For this use-case, the menu will only be opened when pressing and holding the trigger or by using the <kbd>Option</kbd> (Alt on Windows) + <kbd>Down arrow</kbd>/<kbd>Up arrow</kbd> keys while focusing the trigger.
  *
  * This example illustrates the expected visuals and states of the action menu for a trigger with both long press and short press behaviors.
+ *
+ * Please note that the long press functionality is not implemented in this documentation and the example serves only as a visual reference.
  */
 export const LongPress = Template.bind({});
 LongPress.args = {
@@ -165,7 +162,7 @@ LongPress.parameters = {
 };
 
 /**
- * Action menus can be positioned in four locals relative to the trigger but <u>only one menu can be triggered at a single time</u>.
+ * Action menus can be positioned in four locals relative to the trigger but <em>only one menu can be triggered at a single time</em>.
  */
 export const PlacementOptions = (args, context) => ArgGrid({
 	Template,

components/actionmenu/stories/template.js

@@ -3,6 +3,8 @@ import { Template as Menu } from "@spectrum-css/menu/stories/template.js";
 import { Template as Popover } from "@spectrum-css/popover/stories/template.js";
 import { getRandomId } from "@spectrum-css/preview/decorators";
 
+import "../index.css";
+
 export const Template = (
 	{
 		rootClass = "spectrum-ActionMenu",
@@ -22,8 +24,6 @@ export const Template = (
 	} = {},
 	context = {},
 ) => {
-	const { updateArgs } = context;
-
 	return Popover(
 		{
 			...popoverArgs,
@@ -40,12 +40,9 @@ export const Template = (
 						hasPopup: "menu",
 						hasLongPress,
 						id: triggerId,
+						isOpen,
+						isSelected: isOpen,
 						customClasses: [`${rootClass}-trigger`],
-						onclick: hasLongPress
-							? undefined
-							: () => {
-								updateArgs({ isOpen: !isOpen });
-							},
 					},
 					context,
 				),

components/coachmark/stories/coachmark.test.js

@@ -48,12 +48,10 @@ export const CoachMarkGroup = Variants({
 			testHeading: "With action menu",
 			hasPagination: false,
 			hasActionMenu: true,
-			isOpen: true,
 		},
 		{
 			testHeading: "With action menu + media",
 			hasPagination: false,
-			isOpen: true,
 			hasImage: true,
 			hasActionMenu: true,
 		},

components/coachmark/stories/template.js

@@ -1,4 +1,4 @@
-import { Template as ActionMenu } from "@spectrum-css/actionmenu/stories/template.js";
+import { Template as ActionButton } from "@spectrum-css/actionbutton/stories/template.js";
 import { Template as ButtonGroup } from "@spectrum-css/buttongroup/stories/template.js";
 import { Template as CoachIndicator } from "@spectrum-css/coachindicator/stories/template.js";
 import { Template as Popover } from "@spectrum-css/popover/stories/template.js";
@@ -22,7 +22,6 @@ export const CoachContainer = (
 		content = "Pixel brushes use pixels to create brush strokes, just like in other design and drawing tools. Start drawing, and zoom in to see the pixels in each stroke.",
 		currentStep = 2,
 		totalStepCount = 8,
-		isOpen = false,
 		alt = "",
 	} = {},
 	context = {},
@@ -52,33 +51,19 @@ export const CoachContainer = (
 		<div class="spectrum-CoachMark-header" style=${styleMap({
 			"--mod-popover-width": "0px",
 			"--mod-popover-height": "0px",
-			"--mod-popover-wrapper-spacing": "0px",
+			"--spectrum-popover-animation-distance": "0px",
 		})}>
 			<div class="spectrum-CoachMark-title">${title}</div>
+			<!-- This only demonstrates the presence of the action button but not the popover menu -->
 			${when(
 				hasActionMenu,
-				() => ActionMenu(
+				() => ActionButton(
 					{
-						isOpen,
-						position: "bottom-start",
-						triggerArgs: {
-							iconName: "More",
-							size: scale === "large" ? "s" : "m",
-							label: "More actions",
-							hideLabel: true,
-						},
+						iconName: "More",
+						size: scale === "large" ? "s" : "m",
+						label: "More actions",
+						hideLabel: true,
 						customClasses: [`${rootClass}-action-menu`],
-						menuArgs: {
-							size: scale === "large" ? "s" : "m",
-							items: [
-								{
-									label: "Skip tour",
-								},
-								{
-									label: "Reset tour",
-								},
-							],
-						},
 					},
 					context,
 				),
@@ -179,7 +164,6 @@ export const CoachmarkMenuStatesTemplate = (args, context) =>
 						...args,
 						hasImage: false,
 						hasActionMenu: true,
-						isOpen: true
 					},
 					context,
 				),