chore(popover): adjust/enhance popover spacing by default (#4174)

Author: castastrophe

.changeset/modern-times-happen.md

@@ -0,0 +1,5 @@
+---
+"@spectrum-css/popover": patch
+---
+
+Popover positioning "top-left" and "top-right" were using inline properties which caused the spacings to swap in RTL mode. This was unintended as the "right" and "left" naming conventions are meant to retain their location in LTR or RTL mode.

components/actionbar/stories/template.js

@@ -74,8 +74,6 @@ export const Template = ({
 						],
 					}, context),
 				],
-				popoverHeight: 42,
-				popoverWidth: 500,
 			}, context)}
 		</div>
 	`;

components/actionmenu/stories/template.js

@@ -33,10 +33,10 @@ export const Template = ({
 				iconName,
 				iconSet,
 				id: triggerId,
-				customClasses,
 			}, context),
 		position: "bottom-start",
-		customStyles,
+		customWrapperClasses: customClasses,
+		customWrapperStyles: customStyles,
 		content: [
 			(passthroughs) => Menu({
 				...passthroughs,

components/alertdialog/stories/alertdialog.stories.js

@@ -68,11 +68,6 @@ export default {
 			type: "figma",
 			url: "https://www.figma.com/design/Mngz9H7WZLbrCvGQf3GnsY/S2---Desktop?node-id=21917-157",
 		},
-		docs: {
-			story: {
-				height: "300px",
-			}
-		},
 		packageJson,
 		metadata,
 		status: {
@@ -119,11 +114,6 @@ Information.args = {
 	content: "If you enjoy our app, would you mind taking a moment to rate it?",
 };
 Information.parameters = {
-	docs: {
-		story: {
-			height: "400px",
-		}
-	},
 	chromatic: { disableSnapshot: true },
 };
 
@@ -187,11 +177,6 @@ Overflow.args = {
 };
 Overflow.parameters = {
 	chromatic: { disableSnapshot: true },
-	docs: {
-		story: {
-			height: "525px",
-		},
-	},
 };
 
 /**

components/coachindicator/stories/template.js

@@ -1,5 +1,7 @@
+import { getRandomId } from "@spectrum-css/preview/decorators";
 import { html } from "lit-html";
 import { classMap } from "lit-html/directives/class-map.js";
+import { ifDefined } from "lit-html/directives/if-defined.js";
 import { styleMap } from "lit-html/directives/style-map.js";
 
 import "../index.css";
@@ -10,6 +12,7 @@ export const Template = ({
 	staticColor,
 	customClasses = [],
 	customStyles = {},
+	id = getRandomId("coach-indicator"),
 } = {}) => {
 	return html`
 		<div
@@ -21,6 +24,7 @@ export const Template = ({
 				...customClasses.reduce((a, c) => ({ ...a, [c]: true }), {}),
 			})}
 			style=${styleMap(customStyles)}
+			id=${ifDefined(id)}
 		>
 			${Array.from({ length: 3 }).map(() => html`
 				<div class=${classMap({ [`${rootClass}-ring`]: true })}></div>

components/coachmark/stories/coachmark.stories.js

@@ -94,11 +94,6 @@ export default {
 		},
 		packageJson,
 		metadata,
-		docs: {
-			story: {
-				height: "525px",
-			},
-		},
 		status: {
 			type: "migrated",
 		},
@@ -121,11 +116,6 @@ Standard.parameters = {
 	chromatic: {
 		disableSnapshot: true,
 	},
-	docs: {
-		story: {
-			height: "475px",
-		},
-	},
 };
 Standard.args = {
 	imageSource: "example-card-landscape.png",
@@ -141,11 +131,6 @@ StandardNoMedia.parameters = {
 	chromatic: {
 		disableSnapshot: true,
 	},
-	docs: {
-		story: {
-			height: "250px",
-		},
-	},
 };
 
 /** Images and media have a minimum height and can grow with the parent component. Fixed height media is constrained to a 4:3 aspect ratio by applying the `spectrum-CoachMark-image-wrapper--fixedHeight` class. When this fixed height class is used, the height can be customized using the modifiable custom property `--mod-coachmark-media-fixed-height`.  */
@@ -155,11 +140,6 @@ MediaOptions.args = {
 	imageSource: "example-card-portrait.png",
 };
 MediaOptions.parameters = {
-	docs: {
-		story: {
-			height: "725px",
-		},
-	},
 	chromatic: {
 		disableSnapshot: true,
 	},

components/coachmark/stories/template.js

@@ -47,32 +47,34 @@ export const CoachContainer = (
 				</div>
 			`,
 		)}
-		<div class="spectrum-CoachMark-header">
+		<div class="spectrum-CoachMark-header" style=${styleMap({
+			"--mod-popover-width": "0px",
+			"--mod-popover-height": "0px",
+			"--mod-popover-wrapper-spacing": "0px",
+		})}>
 			<div class="spectrum-CoachMark-title">${title}</div>
 			${when(
 				hasActionMenu,
-				() =>
-					html` <div class="spectrum-CoachMark-action-menu">
-						${ActionMenu(
+				() => ActionMenu(
+					{
+						isOpen,
+						position: "bottom-start",
+						iconName: "More",
+						size: scale === "large" ? "s" : "m",
+						customClasses: [
+							`${rootClass}-action-menu`
+						],
+						items: [
+							{
+								label: "Skip tour",
+							},
 							{
-								isOpen,
-								position: "bottom-start",
-								iconName: "More",
-								size: scale === "large" ? "s" : "m",
-								items: [
-									{
-										label: "Skip tour",
-									},
-									{
-										label: "Reset tour",
-									},
-								],
-								popoverHeight: 68,
-								popoverWidth: 84,
+								label: "Reset tour",
 							},
-							context,
-						)}
-					</div>`,
+						],
+					},
+					context,
+				),
 			)}
 		</div>
 		<div class="spectrum-CoachMark-content">
@@ -127,29 +129,24 @@ export const CoachContainer = (
 };
 
 export const Template = (args, context) => {
-	return html`
-		<div
-			class=${classMap({
-				[args.rootClass]: true,
-				...args.customClasses.reduce((a, c) => ({ ...a, [c]: true }), {}),
-			})}
-			style=${styleMap(args.customStyles)}
-		>
-			${Popover(
-				{
-					customStyles: {
-						"inline-size": "var(--spectrum-coach-mark-width)",
-					},
-					customClasses: [`${args.rootClass}-popover`],
-					isOpen: true,
-					position: "right-top",
-					trigger: (passthrough) => CoachIndicator(passthrough, context),
-					content: [CoachContainer(args, context)],
-				},
-				context,
-			)}
-		</div>
-	`;
+	return Popover(
+		{
+			customWrapperClasses: [
+				args.rootClass,
+				...args?.customClasses ?? []
+			],
+			customStyles: {
+				...args?.customStyles ?? {},
+				"inline-size": "var(--spectrum-coach-mark-width)",
+			},
+			customClasses: [`${args.rootClass}-popover`],
+			isOpen: true,
+			position: "right-top",
+			trigger: (passthrough) => CoachIndicator(passthrough, context),
+			content: [CoachContainer(args, context)],
+		},
+		context,
+	);
 };
 
 /* Displays open and closed action menus in a single story. */
@@ -165,15 +162,7 @@ export const CoachmarkMenuStatesTemplate = (args, context) =>
 			Container({
 				withBorder: false,
 				heading: "With action menu (closed) and media",
-				content: Template(
-					{
-						...args,
-						customStyles: {
-							"height": "265px"
-						}
-					},
-					context,
-				),
+				content: Template(args, context),
 			}),
 			Container({
 				withBorder: false,
@@ -183,10 +172,7 @@ export const CoachmarkMenuStatesTemplate = (args, context) =>
 						...args,
 						hasImage: false,
 						hasActionMenu: true,
-						isOpen: true,
-						customStyles: {
-							"height": "260px"
-						}
+						isOpen: true
 					},
 					context,
 				),

components/combobox/stories/combobox.stories.js

@@ -211,11 +211,6 @@ DefaultGroup.args = {
 DefaultGroup.tags = ["!dev"];
 DefaultGroup.parameters = {
 	chromatic: { disableSnapshot: true },
-	docs: {
-		story: {
-			height: "360px",
-		},
-	},
 };
 
 /**

components/combobox/stories/combobox.test.js

@@ -16,9 +16,6 @@ export const ComboBoxGroup = Variants({
 		{
 			testHeading: "Open",
 			isOpen: true,
-			wrapperStyles: {
-				"min-block-size": "250px",
-			},
 		},
 		{
 			testHeading: "Help text with label",

components/combobox/stories/template.js

@@ -56,9 +56,6 @@ export const Template = ({
 		}, 0);
 	}
 
-	const popoverHeight = size === "s" ? 106 : size === "l" ? 170 : size === "xl" ? 229 : 142; // default value is "m"
-	const adjustedPopoverHeight = showFieldLabel ? popoverHeight : popoverHeight + 32; // Subtract label height when no label
-
 	return html`
 		<div
 			class=${classMap({
@@ -78,10 +75,7 @@ export const Template = ({
 			})}
 			id=${ifDefined(id)}
 			data-testid=${ifDefined(testId ?? id)}
-			style=${styleMap({
-				...customStyles,
-				["margin-block-end"]: !isReadOnly && isOpen && !isDisabled ? `${adjustedPopoverHeight}px` : undefined,
-			})}
+			style=${styleMap(customStyles)}
 			role="combobox"
 			aria-expanded=${isOpen}
 			aria-haspopup="listbox"
@@ -150,7 +144,6 @@ export const Template = ({
 					"inline-size": size === "s" ? "192px" : size === "l" ? "224px" : size === "xl" ? "240px" : "208px",
 				},
 				content,
-				popoverHeight,
 			}, context)}
 			${when(helpText, () =>
 				HelpText({