mirror of
https://github.com/vector-im/element-call.git
synced 2026-09-22 22:29:30 +00:00
Draw the remove button as design drew it
- Critical from the moment it appears, not only under the pointer: the ground carries the hover instead, and gains a ring where the shadow was. - 28px on the tile's corner, overhanging it by a fifth of itself, with the glyph a third of the button as the mock draws it. - It names itself: a Remove tooltip, so the cross has words. - Every colour is a Compound token and lands on the mock's own value in both themes, bar the dark hover ground, which is one step up the red ramp from what the mock measures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -330,27 +330,34 @@ Please see LICENSE in the repository root for full details.
|
|||||||
tile, so the one destructive action lived where the ordinary one does. */
|
tile, so the one destructive action lived where the ordinary one does. */
|
||||||
inset-block-start: 0;
|
inset-block-start: 0;
|
||||||
inset-inline-end: 0;
|
inset-inline-end: 0;
|
||||||
translate: 30% -30%;
|
/* It overhangs the corner by about a fifth of itself, as the design draws
|
||||||
|
it: enough to read as sitting on the tile rather than in it, little
|
||||||
|
enough that it never reaches the tile beside it. */
|
||||||
|
translate: 20% -20%;
|
||||||
display: none;
|
display: none;
|
||||||
align-items: center;
|
align-items: center;
|
||||||
justify-content: center;
|
justify-content: center;
|
||||||
inline-size: 22px;
|
inline-size: 28px;
|
||||||
block-size: 22px;
|
block-size: 28px;
|
||||||
border-radius: 50%;
|
border-radius: 50%;
|
||||||
background: var(--cpd-color-bg-canvas-default);
|
background: var(--cpd-color-bg-canvas-default);
|
||||||
color: var(--cpd-color-icon-primary);
|
/* Red at rest, not only under the pointer. The design says what the button
|
||||||
box-shadow: 0 0 0 var(--cpd-border-width-1)
|
does before you are on it, and the ground carries the hover instead. */
|
||||||
var(--cpd-color-border-interactive-secondary);
|
color: var(--cpd-color-icon-critical-primary);
|
||||||
|
/* Raised off the tile rather than outlined against it — Compound's own value
|
||||||
|
for a small raised surface, there being no shadow token. In a dark theme
|
||||||
|
it falls on a near-black ground and reads as nothing, which is how the
|
||||||
|
design draws it there too. */
|
||||||
|
box-shadow: 0 1.2px 2.4px 0 rgb(0 0 0 / 15%);
|
||||||
cursor: pointer;
|
cursor: pointer;
|
||||||
}
|
}
|
||||||
|
|
||||||
/* Red under the pointer: the corner separates it from choosing the tile, and
|
/* Under the pointer the ground turns critical and the shadow gives way to a
|
||||||
this says what it will do once you are there. */
|
ring, so the button reads as pressable and as destructive at once. */
|
||||||
.effectRemove:hover {
|
.effectRemove:hover {
|
||||||
background: var(--cpd-color-bg-critical-subtle);
|
background: var(--cpd-color-bg-critical-subtle-hovered);
|
||||||
color: var(--cpd-color-icon-critical-primary);
|
|
||||||
box-shadow: 0 0 0 var(--cpd-border-width-1)
|
box-shadow: 0 0 0 var(--cpd-border-width-1)
|
||||||
var(--cpd-color-border-critical-primary);
|
var(--cpd-color-border-critical-subtle);
|
||||||
}
|
}
|
||||||
|
|
||||||
.effectGrid .effectTileWrap:hover .effectRemove,
|
.effectGrid .effectTileWrap:hover .effectRemove,
|
||||||
|
|||||||
@@ -7,6 +7,7 @@ Please see LICENSE in the repository root for full details.
|
|||||||
|
|
||||||
import { fn, userEvent, waitFor, within, expect } from "storybook/test";
|
import { fn, userEvent, waitFor, within, expect } from "storybook/test";
|
||||||
import { useEffect, useState, type FC, type JSX, type ReactNode } from "react";
|
import { useEffect, useState, type FC, type JSX, type ReactNode } from "react";
|
||||||
|
import { TooltipProvider } from "@vector-im/compound-web";
|
||||||
|
|
||||||
import type { Meta, StoryObj } from "@storybook/react-vite";
|
import type { Meta, StoryObj } from "@storybook/react-vite";
|
||||||
import { MediaMuteAndSwitchButton } from "./MediaMuteAndSwitchButton";
|
import { MediaMuteAndSwitchButton } from "./MediaMuteAndSwitchButton";
|
||||||
@@ -105,14 +106,18 @@ const WithACallArea: FC<{ children: ReactNode }> = ({ children }) => {
|
|||||||
const meta = {
|
const meta = {
|
||||||
component: MediaMuteAndSwitchButton,
|
component: MediaMuteAndSwitchButton,
|
||||||
decorators: [
|
decorators: [
|
||||||
|
// The app puts one of these over everything; the remove cross needs it to
|
||||||
|
// be able to name itself.
|
||||||
(Story): JSX.Element => (
|
(Story): JSX.Element => (
|
||||||
<MediaDevicesContext value={mediaDevices}>
|
<TooltipProvider>
|
||||||
<WithACallArea>
|
<MediaDevicesContext value={mediaDevices}>
|
||||||
<WithAMicrophone>
|
<WithACallArea>
|
||||||
<Story />
|
<WithAMicrophone>
|
||||||
</WithAMicrophone>
|
<Story />
|
||||||
</WithACallArea>
|
</WithAMicrophone>
|
||||||
</MediaDevicesContext>
|
</WithACallArea>
|
||||||
|
</MediaDevicesContext>
|
||||||
|
</TooltipProvider>
|
||||||
),
|
),
|
||||||
],
|
],
|
||||||
} satisfies Meta<typeof MediaMuteAndSwitchButton>;
|
} satisfies Meta<typeof MediaMuteAndSwitchButton>;
|
||||||
@@ -993,6 +998,42 @@ export const AddedBackgroundsCanBeRemoved: Story = {
|
|||||||
await expect(args.onRemoveBackgroundEffect).toHaveBeenCalledWith(
|
await expect(args.onRemoveBackgroundEffect).toHaveBeenCalledWith(
|
||||||
"added:two",
|
"added:two",
|
||||||
);
|
);
|
||||||
|
|
||||||
|
// The cross the design drew: critical from the moment it is visible,
|
||||||
|
// rather than only once the pointer is on it, and sitting over the tile's
|
||||||
|
// top-right corner rather than inside it.
|
||||||
|
const wrap = other.closest(`.${styles.effectTileWrap}`)!;
|
||||||
|
await userEvent.hover(wrap);
|
||||||
|
const cross = wrap.querySelector<HTMLElement>(`.${styles.effectRemove}`)!;
|
||||||
|
// Read the token the way the browser will, so the comparison is against
|
||||||
|
// the colour itself and not against how the value happens to be spelled.
|
||||||
|
const token = (name: string): string => {
|
||||||
|
const probe = document.createElement("span");
|
||||||
|
probe.style.color = `var(${name})`;
|
||||||
|
cross.append(probe);
|
||||||
|
const value = getComputedStyle(probe).color;
|
||||||
|
probe.remove();
|
||||||
|
return value;
|
||||||
|
};
|
||||||
|
const rest = getComputedStyle(cross);
|
||||||
|
await expect(rest.color).toBe(token("--cpd-color-icon-critical-primary"));
|
||||||
|
const tile = other.getBoundingClientRect();
|
||||||
|
const box = cross.getBoundingClientRect();
|
||||||
|
await expect(box.right).toBeGreaterThan(tile.right);
|
||||||
|
await expect(box.top).toBeLessThan(tile.top);
|
||||||
|
|
||||||
|
// And it names itself once the pointer is on it. The ground turning
|
||||||
|
// critical under the pointer is not asserted here: a story drives the
|
||||||
|
// pointer with synthetic events, which never raise CSS hover — that one is
|
||||||
|
// measured against the design with a real pointer instead.
|
||||||
|
await userEvent.hover(cross);
|
||||||
|
await waitFor(async () =>
|
||||||
|
expect(
|
||||||
|
[...document.body.querySelectorAll("div")].some(
|
||||||
|
(d) => d.textContent === "Remove" && d.offsetParent !== null,
|
||||||
|
),
|
||||||
|
).toBe(true),
|
||||||
|
);
|
||||||
},
|
},
|
||||||
};
|
};
|
||||||
|
|
||||||
|
|||||||
@@ -21,6 +21,7 @@ import {
|
|||||||
MenuItem,
|
MenuItem,
|
||||||
MenuTitle,
|
MenuTitle,
|
||||||
RadioInput,
|
RadioInput,
|
||||||
|
Tooltip,
|
||||||
ToggleMenuItem,
|
ToggleMenuItem,
|
||||||
} from "@vector-im/compound-web";
|
} from "@vector-im/compound-web";
|
||||||
import {
|
import {
|
||||||
@@ -582,19 +583,25 @@ export const MediaMuteAndSwitchButton: FC<MediaMuteAndSwitchButtonProps> = ({
|
|||||||
return canRemove(effect) ? (
|
return canRemove(effect) ? (
|
||||||
<div className={styles.effectTileWrap} key={effect.id}>
|
<div className={styles.effectTileWrap} key={effect.id}>
|
||||||
{tile}
|
{tile}
|
||||||
{/* eslint-disable-next-line jsx-a11y/click-events-have-key-events,
|
{/* The cross carries no words, so hovering it names what it does.
|
||||||
jsx-a11y/no-static-element-interactions --
|
Interactive as far as the tooltip is concerned — that keeps it
|
||||||
Deliberately not a control: aria-hidden, so assistive technology
|
from wrapping the cross in a tab stop of its own, which is the
|
||||||
never meets it, and the keyboard reaches the same action through
|
one thing this must not grow. */}
|
||||||
Delete on the tile. A role and a tab stop here would add a stop
|
<Tooltip label={t("action.remove")}>
|
||||||
the menu's arrow keys know nothing about. */}
|
{/* eslint-disable-next-line jsx-a11y/click-events-have-key-events,
|
||||||
<span
|
jsx-a11y/no-static-element-interactions --
|
||||||
aria-hidden
|
Deliberately not a control: aria-hidden, so assistive technology
|
||||||
className={styles.effectRemove}
|
never meets it, and the keyboard reaches the same action through
|
||||||
onClick={(): void => onRemoveBackgroundEffect?.(effect.id)}
|
Delete on the tile. A role and a tab stop here would add a stop
|
||||||
>
|
the menu's arrow keys know nothing about. */}
|
||||||
<CloseIcon width={16} height={16} />
|
<span
|
||||||
</span>
|
aria-hidden
|
||||||
|
className={styles.effectRemove}
|
||||||
|
onClick={(): void => onRemoveBackgroundEffect?.(effect.id)}
|
||||||
|
>
|
||||||
|
<CloseIcon width={20} height={20} />
|
||||||
|
</span>
|
||||||
|
</Tooltip>
|
||||||
</div>
|
</div>
|
||||||
) : (
|
) : (
|
||||||
tile
|
tile
|
||||||
|
|||||||
Reference in New Issue
Block a user