From 2b10ae9bcba93f93627d7cede2901e856bf74081 Mon Sep 17 00:00:00 2001 From: Melissa Thompson Date: Mon, 13 May 2024 13:01:58 -0400 Subject: [PATCH 1/5] fix(popover): correct left/right tips for RTL --- components/popover/index.css | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/components/popover/index.css b/components/popover/index.css index a23941955d..2b58ba45a2 100644 --- a/components/popover/index.css +++ b/components/popover/index.css @@ -150,7 +150,8 @@ governing permissions and limitations under the License. /* spacing to include tip in calculation of offset from source */ &.spectrum-Popover--withTip { /* tip size minus where it overlaps with popover border */ - margin-inline-start: calc(var(--mod-popover-pointer-width, var(--spectrum-popover-pointer-width)) - var(--mod-popover-border-width, var(--spectrum-popover-border-width))); + /* stylelint-disable-next-line csstools/use-logical -- intentional, right stays on the same side even for RTL */ + margin-left: calc(var(--mod-popover-pointer-width, var(--spectrum-popover-pointer-width)) - var(--mod-popover-border-width, var(--spectrum-popover-border-width))); } /* popover animates towards right ⮕ */ @@ -166,7 +167,8 @@ governing permissions and limitations under the License. /* spacing to include tip in calculation of offset from source */ &.spectrum-Popover--withTip { /* tip size minus where it overlaps with popover border */ - margin-inline-end: calc(var(--mod-popover-pointer-width, var(--spectrum-popover-pointer-width)) - var(--mod-popover-border-width, var(--spectrum-popover-border-width))); + /* stylelint-disable-next-line csstools/use-logical -- intentional, left stays on the same side even for RTL */ + margin-right: calc(var(--mod-popover-pointer-width, var(--spectrum-popover-pointer-width)) - var(--mod-popover-border-width, var(--spectrum-popover-border-width))); } /* popover animates towards left ⬅ */ @@ -342,7 +344,9 @@ governing permissions and limitations under the License. &.spectrum-Popover--left-bottom, &.spectrum-Popover--left-top { .spectrum-Popover-tip { - inset-inline: 100% auto; + /* stylelint-disable-next-line csstools/use-logical -- intentional, left stays on the same side even for RTL */ + left: 100%; + right: auto; } } @@ -351,7 +355,9 @@ governing permissions and limitations under the License. &.spectrum-Popover--right-bottom, &.spectrum-Popover--right-top { .spectrum-Popover-tip { - inset-inline: auto 100%; + /* stylelint-disable-next-line csstools/use-logical -- intentional, right stays on the same side even for RTL */ + right: 100%; + left: auto; /* flip tip to point left ◁ */ transform: scaleX(-1); From 2b0c75dd0d84175fc93987aff675b2bd2f7c6e8f Mon Sep 17 00:00:00 2001 From: Melissa Thompson Date: Mon, 13 May 2024 10:58:22 -0400 Subject: [PATCH 2/5] docs(popover): show all placement options --- components/popover/stories/popover.stories.js | 40 ++++++++++++------- components/popover/stories/template.js | 34 +++++++++++----- 2 files changed, 49 insertions(+), 25 deletions(-) diff --git a/components/popover/stories/popover.stories.js b/components/popover/stories/popover.stories.js index 0990c12323..e14d0e5f04 100644 --- a/components/popover/stories/popover.stories.js +++ b/components/popover/stories/popover.stories.js @@ -6,6 +6,31 @@ import { Template } from "./template"; import { Template as ActionButton } from "@spectrum-css/actionbutton/stories/template.js"; import { Template as Menu } from "@spectrum-css/menu/stories/template.js"; +const placementOptions = [ + "top", + "top-left", + "top-right", + "top-start", + "top-end", + "bottom", + "bottom-left", + "bottom-right", + "bottom-start", + "bottom-end", + "right", + "right-bottom", + "right-top", + "left", + "left-bottom", + "left-top", + "start", + "start-top", + "start-bottom", + "end", + "end-top", + "end-bottom", +]; + /** * A popover is used to display transient content (menus, options, additional actions etc.) and appears when clicking/tapping on a source (tools, buttons, etc.). It stands out via its visual style (stroke and drop shadow) and floats on top of the rest of the interface. */ @@ -44,20 +69,7 @@ export default { category: "Component", }, control: "select", - options: [ - "top", - "top-start", - "top-end", - "bottom", - "bottom-start", - "bottom-end", - "left", - "left-top", - "left-bottom", - "right", - "right-top", - "right-bottom", - ], + options: placementOptions, if: { arg: "nested", truthy: false }, }, }, diff --git a/components/popover/stories/template.js b/components/popover/stories/template.js index 104eadf62c..6eb9366c89 100644 --- a/components/popover/stories/template.js +++ b/components/popover/stories/template.js @@ -60,49 +60,61 @@ export const Template = ({ const popWidth = popover.offsetWidth ?? 0; const popHeight = popover.offsetHeight ?? 0; const textDir = getComputedStyle(document.querySelector("html")).direction; + let x, y; let xOffset = "+ 0px"; let yOffset = "+ 0px"; - if (position.includes("top") || position.includes("bottom") && !(position.includes("-top") || position.includes("-bottom"))) { + + if (position.startsWith("top") || position.startsWith("bottom")) { x = triggerXCenter - (popWidth > 0 ? popWidth / 2 : popWidth); } - if (position.includes("left") || position.includes("right")) { + if (position.includes("left") || position.includes("right") || position.startsWith("start") || position.startsWith("end")) { y = triggerYCenter - (popHeight > 0 ? popHeight / 2 : popHeight); } - if (position.includes("top") && !position.includes("-top")) { + if (position.startsWith("top")) { y = rect.top - popHeight; yOffset = withTip ? "- (var(--spectrum-popover-pointer-height) + var(--spectrum-popover-animation-distance) - 1px)" : "- var(--spectrum-popover-animation-distance)"; } - else if (position.includes("bottom") && !position.includes("-bottom")) { + else if (position.startsWith("bottom")) { y = rect.bottom; yOffset = "+ (var(--spectrum-popover-animation-distance))"; } - else if (position.includes("left")) { + else if (position.includes("left")) { if (textDir == "rtl") { x = rect.right; xOffset = withTip ? "+ 0px" : "+ var(--spectrum-popover-animation-distance)"; } - else { + else { x = rect.left - popWidth; xOffset = withTip ? "- ((var(--spectrum-popover-pointer-width) / 2) + var(--spectrum-popover-animation-distance) - 2px)" : "- var(--spectrum-popover-animation-distance)"; } } - else if (position.includes("right")) { + else if (position.includes("right")) { if (textDir == "rtl") { x = rect.left - popWidth; xOffset = withTip ? "- ((var(--spectrum-popover-pointer-width) / 2) + var(--spectrum-popover-animation-distance) - 2px)" : "- var(--spectrum-popover-animation-distance)"; } - else { + else { x = rect.right; xOffset = withTip ? "+ 0px" : "+ var(--spectrum-popover-animation-distance)"; } } + else if (position.includes("start")) { + x = rect.left - popWidth; + xOffset = withTip + ? "- ((var(--spectrum-popover-pointer-width) / 2) + var(--spectrum-popover-animation-distance) - 2px)" + : "- var(--spectrum-popover-animation-distance)"; + } + else if (position.includes("end")) { + x = rect.right; + xOffset = withTip ? "+ 0px" : "+ var(--spectrum-popover-animation-distance)"; + } if (x) transforms.push(`translateX(calc(var(--flow-direction) * calc(${parseInt(x, 10)}px ${xOffset})))`); if (y) transforms.push(`translateY(calc(${y}px ${yOffset}))`); @@ -111,13 +123,13 @@ export const Template = ({ if (position === "top-start" || position === "bottom-start") { additionalStyles["inset-inline-start"] = "calc(" + (popWidth / 2) + "px - var(--spectrum-popover-pointer-edge-offset))"; } - else if (position === "top-end" || position === "bottom-end") { + else if (position === "top-end" || position === "bottom-end") { additionalStyles["inset-inline-start"] = "calc(-1 *" + (popWidth / 2) + "px + var(--spectrum-popover-pointer-edge-offset))"; } - else if (position === "left-top" || position === "right-top") { + else if (position === "left-top" || position === "right-top" || position === "start-top" || position === "end-top") { additionalStyles["inset-block-start"] = "calc(" + (popHeight / 2) + "px - var(--spectrum-popover-pointer-edge-offset))"; } - else if (position === "left-bottom" || position === "right-bottom") { + else if (position === "left-bottom" || position === "right-bottom" || position === "start-bottom" || position === "end-bottom") { additionalStyles["inset-block-start"] = "calc(-1 *" + (popHeight / 2) + "px + var(--spectrum-popover-pointer-edge-offset))"; } From 8efab7a53d45f9da0e421b11c69dc3a706e4d2f0 Mon Sep 17 00:00:00 2001 From: Melissa Thompson Date: Mon, 13 May 2024 13:02:26 -0400 Subject: [PATCH 3/5] docs(popover): increase chromatic coverage for tip position --- components/popover/stories/popover.stories.js | 57 ++++++++++++++++++- components/popover/stories/template.js | 36 ++++++++++++ 2 files changed, 90 insertions(+), 3 deletions(-) diff --git a/components/popover/stories/popover.stories.js b/components/popover/stories/popover.stories.js index e14d0e5f04..0f21879a50 100644 --- a/components/popover/stories/popover.stories.js +++ b/components/popover/stories/popover.stories.js @@ -1,7 +1,9 @@ +import { Template as Typography } from "@spectrum-css/typography/stories/template.js"; import { userEvent, within } from "@storybook/testing-library"; import { html } from "lit"; +import { when } from "lit/directives/when.js"; -import { Template } from "./template"; +import { SourcelessTemplate, Template } from "./template"; import { Template as ActionButton } from "@spectrum-css/actionbutton/stories/template.js"; import { Template as Menu } from "@spectrum-css/menu/stories/template.js"; @@ -125,10 +127,57 @@ export default { }, }, decorators: [ - (Story, context) => html`
${Story(context)}
` + (Story, context) => html` + +
${Story(context)}
+ `, ], }; + +const ChromaticTipPlacementVariants = (args) => html` + ${placementOptions.map(option => { + const optionDescription = () => { + if (option.startsWith("start") || option.startsWith("end")) + return "Changes side with text direction (like a logical property)"; + if (option.startsWith("left") || option.startsWith("right")) + return "Text direction does not effect the position"; + return null; + }; + + return html` +
+ ${Typography({ + semantics: "detail", + size: "l", + content: [`${option}`], + })} +
+ ${when(optionDescription() !== null, () => html` + ${Typography({ + semantics: "detail", + size: "s", + content: [`${optionDescription()}`], + })} + `)} +
+
+ ${SourcelessTemplate({ + ...args, + position: option, + })} +
+
+ `; + })} +`; + export const Default = Template.bind({}); Default.play = async ({ canvasElement }) => { const canvas = within(canvasElement); @@ -136,7 +185,9 @@ Default.play = async ({ canvasElement }) => { }; Default.args = {}; -export const WithTip = Template.bind({}); +export const WithTip = (args) => html` + ${window.isChromatic() ? ChromaticTipPlacementVariants(args) : Template(args)} +`; WithTip.play = async ({ canvasElement }) => { const canvas = within(canvasElement); await userEvent.click(canvas.getByRole("button")); diff --git a/components/popover/stories/template.js b/components/popover/stories/template.js index 6eb9366c89..48555bc9d8 100644 --- a/components/popover/stories/template.js +++ b/components/popover/stories/template.js @@ -171,3 +171,39 @@ export const Template = ({ `; }; + +export const SourcelessTemplate = ({ + rootClass = "spectrum-Popover", + size = "m", + isOpen = false, + withTip = false, + position = "top", + customClasses = [], + id = "popover-1", + testId, + customStyles = {}, + content = [], +}) => html` +
({ ...a, [c]: true }), {}), + })} + style=${ifDefined(styleMap(customStyles))} + role="presentation" + id=${ifDefined(id)} + data-testid=${ifDefined(testId)} + > + ${content.map((c) => (typeof c === "function" ? c({}) : c))} + ${withTip + ? position && ["top", "bottom"].some((e) => position.startsWith(e)) + ? html`` + : html`` + : ""} +
+`; From 2c5aac68a545e6b0abc99252a78517829009eb37 Mon Sep 17 00:00:00 2001 From: Melissa Thompson Date: Mon, 13 May 2024 13:14:39 -0400 Subject: [PATCH 4/5] chore: changeset, grammar --- .changeset/three-geese-relax.md | 7 +++++++ components/popover/stories/popover.stories.js | 7 ++++--- 2 files changed, 11 insertions(+), 3 deletions(-) create mode 100644 .changeset/three-geese-relax.md diff --git a/.changeset/three-geese-relax.md b/.changeset/three-geese-relax.md new file mode 100644 index 0000000000..de375e3e10 --- /dev/null +++ b/.changeset/three-geese-relax.md @@ -0,0 +1,7 @@ +--- +"@spectrum-css/popover": patch +--- + +fix(popover): correct left/right tips for RTL + +Includes exposing Start and End tip placement variants in Storybook, and increased VRT coverage for Chromatic. diff --git a/components/popover/stories/popover.stories.js b/components/popover/stories/popover.stories.js index 0f21879a50..26ff3739bb 100644 --- a/components/popover/stories/popover.stories.js +++ b/components/popover/stories/popover.stories.js @@ -147,7 +147,7 @@ const ChromaticTipPlacementVariants = (args) => html` if (option.startsWith("start") || option.startsWith("end")) return "Changes side with text direction (like a logical property)"; if (option.startsWith("left") || option.startsWith("right")) - return "Text direction does not effect the position"; + return "Text direction does not affect the position"; return null; }; @@ -166,11 +166,12 @@ const ChromaticTipPlacementVariants = (args) => html` content: [`${optionDescription()}`], })} `)} - +
${SourcelessTemplate({ ...args, position: option, + isOpen: true, })}
@@ -190,7 +191,7 @@ export const WithTip = (args) => html` `; WithTip.play = async ({ canvasElement }) => { const canvas = within(canvasElement); - await userEvent.click(canvas.getByRole("button")); + window.isChromatic() ? null : await userEvent.click(canvas.getByRole("button")); }; WithTip.args = { withTip: true, From fe34cdf49f9cf26bf22ca290b164024222c0bd1b Mon Sep 17 00:00:00 2001 From: Melissa Thompson Date: Wed, 22 May 2024 15:39:16 -0400 Subject: [PATCH 5/5] fix: remove duplicate template code --- components/popover/stories/popover.stories.js | 5 +-- components/popover/stories/template.js | 36 ------------------- 2 files changed, 3 insertions(+), 38 deletions(-) diff --git a/components/popover/stories/popover.stories.js b/components/popover/stories/popover.stories.js index 26ff3739bb..37c446b03f 100644 --- a/components/popover/stories/popover.stories.js +++ b/components/popover/stories/popover.stories.js @@ -3,7 +3,7 @@ import { userEvent, within } from "@storybook/testing-library"; import { html } from "lit"; import { when } from "lit/directives/when.js"; -import { SourcelessTemplate, Template } from "./template"; +import { Template } from "./template"; import { Template as ActionButton } from "@spectrum-css/actionbutton/stories/template.js"; import { Template as Menu } from "@spectrum-css/menu/stories/template.js"; @@ -168,10 +168,11 @@ const ChromaticTipPlacementVariants = (args) => html` `)}
- ${SourcelessTemplate({ + ${Template({ ...args, position: option, isOpen: true, + trigger: () => null, })}
diff --git a/components/popover/stories/template.js b/components/popover/stories/template.js index 48555bc9d8..6eb9366c89 100644 --- a/components/popover/stories/template.js +++ b/components/popover/stories/template.js @@ -171,39 +171,3 @@ export const Template = ({ `; }; - -export const SourcelessTemplate = ({ - rootClass = "spectrum-Popover", - size = "m", - isOpen = false, - withTip = false, - position = "top", - customClasses = [], - id = "popover-1", - testId, - customStyles = {}, - content = [], -}) => html` -
({ ...a, [c]: true }), {}), - })} - style=${ifDefined(styleMap(customStyles))} - role="presentation" - id=${ifDefined(id)} - data-testid=${ifDefined(testId)} - > - ${content.map((c) => (typeof c === "function" ? c({}) : c))} - ${withTip - ? position && ["top", "bottom"].some((e) => position.startsWith(e)) - ? html`` - : html`` - : ""} -
-`;