Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(popover): correct left/right tips for RTL, increase VRT coverage #2753

Merged
merged 5 commits into from
May 22, 2024
Merged

fix(popover): correct left/right tips for RTL, increase VRT coverage #2753

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2b10ae9
fix(popover): correct left/right tips for RTL
mdt2 May 13, 2024
2b0c75d
docs(popover): show all placement options
mdt2 May 13, 2024
8efab7a
docs(popover): increase chromatic coverage for tip position
mdt2 May 13, 2024
2c5aac6
chore: changeset, grammar
mdt2 May 13, 2024
fe34cdf
fix: remove duplicate template code
mdt2 May 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/three-geese-relax.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@spectrum-css/popover": patch
---

fix(popover): correct left/right tips for RTL
mdt2 marked this conversation as resolved.
Show resolved Hide resolved

Includes exposing Start and End tip placement variants in Storybook, and increased VRT coverage for Chromatic.
14 changes: 10 additions & 4 deletions components/popover/index.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
castastrophe marked this conversation as resolved.
Show resolved Hide resolved
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 ⮕ */
Expand All @@ -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 ⬅ */
Expand Down Expand Up @@ -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;
}
}

Expand All @@ -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);
Expand Down
99 changes: 82 additions & 17 deletions components/popover/stories/popover.stories.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,38 @@
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 { 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.
*/
Expand Down Expand Up @@ -44,20 +71,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 },
},
},
Expand Down Expand Up @@ -113,21 +127,72 @@ export default {
},
},
decorators: [
(Story, context) => html`<div style="padding: 16px">${Story(context)}</div>`
(Story, context) => html`
<style>
.spectrum-Detail { display: inline-block; }
.spectrum-Typography > div {
/* Why seafoam? Because it separates it from the component styles. */
--mod-detail-font-color: var(--spectrum-seafoam-900);
}
</style>
<div style="padding: 16px">${Story(context)}</div>
`,
],
};


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 affect the position";
return null;
};

return html`
<div class="spectrum-Typography" style="padding-bottom: 12rem;">
${Typography({
semantics: "detail",
size: "l",
content: [`${option}`],
})}
<div>
${when(optionDescription() !== null, () => html`
${Typography({
semantics: "detail",
size: "s",
content: [`${optionDescription()}`],
})}
`)}
</div>
<div style="padding-top: 2rem">
${Template({
...args,
position: option,
isOpen: true,
trigger: () => null,
})}
</div>
</div>
`;
})}
`;

export const Default = Template.bind({});
Default.play = async ({ canvasElement }) => {
const canvas = within(canvasElement);
await userEvent.click(canvas.getByRole("button"));
};
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"));
window.isChromatic() ? null : await userEvent.click(canvas.getByRole("button"));
};
WithTip.args = {
withTip: true,
Expand Down
34 changes: 23 additions & 11 deletions components/popover/stories/template.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}))`);
Expand All @@ -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))";
}

Expand Down
Loading