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/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,20 @@ export const Template = ({
 	role = "button",
 } = {}, context = {}) => {
 	const { updateArgs } = context;
+
+	// If a custom onclick handler isn't provided, close the popover when clicking outside of the button
+	if (typeof onclick !== "function") {
+		document.body.addEventListener("click", function (evt) {
+			if (evt.target.closest(`.${rootClass}`)) {
+				return;
+			}
+			updateArgs({
+				isSelected: false,
+				isOpen: false,
+			});
+		});
+	}
+
 	return html`
 		<button
 			aria-label=${ifDefined(hideLabel ? label : undefined)}
@@ -77,31 +91,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",
@@ -24,6 +26,15 @@ export const Template = (
 ) => {
 	const { updateArgs } = context;
 
+	document.body.addEventListener("click", function (evt) {
+		if (evt.target.closest(`.${rootClass}`)) {
+			return;
+		}
+		updateArgs({
+			isOpen: false,
+		});
+	});
+
 	return Popover(
 		{
 			...popoverArgs,
@@ -40,12 +51,12 @@ export const Template = (
 						hasPopup: "menu",
 						hasLongPress,
 						id: triggerId,
+						isOpen,
+						isSelected: isOpen,
 						customClasses: [`${rootClass}-trigger`],
-						onclick: hasLongPress
-							? undefined
-							: () => {
-								updateArgs({ isOpen: !isOpen });
-							},
+						onclick: () => {
+							updateArgs({ isOpen: !isOpen });
+						},
 					},
 					context,
 				),

components/coachmark/stories/template.js

@@ -52,7 +52,7 @@ 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>
 			${when(

components/popover/stories/template.js

@@ -21,7 +21,7 @@ export const Template = ({
 	if (isOpen) {
 		if (["top", "bottom"].some((e) => position.startsWith(e))) {
 			customWrapperStyles["min-inline-size"] = "var(--spectrum-popover-width)";
-			customWrapperStyles["min-block-size"] = "calc(var(--spectrum-popover-height) + var(--spectrum-popover-trigger-height, 0px) + var(--mod-popover-wrapper-spacing, var(--spectrum-spacing-100) * 2))";
+			customWrapperStyles["min-block-size"] = "calc(var(--spectrum-popover-height) + var(--spectrum-popover-trigger-height, 0px) + var(--spectrum-popover-animation-distance, var(--spectrum-spacing-100) * 2))";
 		}
 		else {
 			customWrapperStyles["min-inline-size"] = "calc(var(--spectrum-popover-width) + var(--spectrum-popover-trigger-width, 0px))";
@@ -95,10 +95,10 @@ export const Popover = ({
 	if (trigger) {
 		// Translate the popover to the correct position, keeping the default spacing between the trigger and the popover in mind.
 		if (position.startsWith("top")) {
-			popoverAlignment["transform"] = "translateY(calc(var(--spectrum-popover-trigger-height, 0px) * -1 - var(--spectrum-spacing-100)))";
+			popoverAlignment["transform"] = "translateY(calc(var(--spectrum-popover-trigger-height, 0px) * -1 - var(--spectrum-popover-animation-distance, var(--spectrum-spacing-100))))";
 		}
 		else if (position.startsWith("bottom")) {
-			popoverAlignment["transform"] = "translateY(calc(var(--spectrum-popover-trigger-height, 0px) + var(--spectrum-spacing-100)))";
+			popoverAlignment["transform"] = "translateY(calc(var(--spectrum-popover-trigger-height, 0px) + var(--spectrum-popover-animation-distance, var(--spectrum-spacing-100))))";
 		}
 
 		// Position the popover to the correct position at the correct side of the trigger.