mirror of
https://github.com/maputnik/editor.git
synced 2026-09-06 04:17:27 +00:00
fix(layer-list): title visibility button with the action it performs (#2135)
- Fixes #1675 ## Problem In the layer list, the show/hide button's `title` tooltip named the layer's current visibility instead of what a click does. A visible layer's button read `show` while clicking it hides the layer, and a hidden layer's button read `hide` while clicking it shows the layer — exactly reversed. `LayerListItem.tsx` derived a single value from the layer state: ```ts const visibilityAction = visibility === "visible" ? "show" : "hide"; ``` and passed it to `IconAction` as `action`, which `IconAction` used for three different things: choosing the icon, building the `--show`/`--hide` CSS modifier, and rendering `title={this.props.action}`. The first two are correct as a state indicator; only the tooltip needs the opposite verb. ## Change - `IconAction` gets an optional `title` prop and falls back to `action` when it is not given, so the delete and duplicate buttons are unchanged. - `LayerListItem` keeps `visibilityAction` (icon + `maputnik-layer-list-icon-action__visibility--hide` CSS modifier, which `_layer.scss` relies on to keep the button visible for hidden layers) and adds `visibilityTitle` for the inverted tooltip. No icon, class name or click behaviour changes. ## Test New regression test in `e2e/layers-list.spec.ts`: it asserts the button is titled `hide` while the layer is visible, clicks it, and asserts it is titled `show` once the layer is hidden. Against the unpatched component the first assertion fails with `Expected: "hide"` / `Received: "show"`, reproducing the reported behaviour. `npm run lint`, `npx tsc --noEmit`, `npx vitest run` (50 tests) and `npx playwright test e2e/layers-list.spec.ts` (28 tests) all pass. ## Note The tooltips in this component (`delete`, `duplicate`, `show`, `hide`) are plain English literals and are not run through `t()` today, so this change keeps them as-is rather than introducing translation keys for one button. Also worth flagging separately: these buttons carry `aria-hidden="true"`, so the `title` never reaches assistive technology at all — out of scope here, but it means the label is a mouse-hover tooltip only.
This commit is contained in:
@@ -1,4 +1,4 @@
|
||||
import { beforeEach, describe, test } from "./utils/fixtures";
|
||||
import { beforeEach, describe, expect, test } from "./utils/fixtures";
|
||||
import { MaputnikDriver } from "./maputnik-driver";
|
||||
|
||||
describe("layers list", () => {
|
||||
@@ -47,6 +47,14 @@ describe("layers list", () => {
|
||||
});
|
||||
});
|
||||
|
||||
test("should title the visibility button with the action it performs", async () => {
|
||||
const key = "layer-list-item:" + id + ":toggle-visibility";
|
||||
// A visible layer is about to be hidden, and a hidden one to be shown.
|
||||
await expect(get.elementByTestId(key)).toHaveAttribute("title", "hide");
|
||||
await when.click(key);
|
||||
await expect(get.elementByTestId(key)).toHaveAttribute("title", "show");
|
||||
});
|
||||
|
||||
describe("when clicking hide", () => {
|
||||
beforeEach(async () => {
|
||||
await when.click("layer-list-item:" + id + ":toggle-visibility");
|
||||
|
||||
@@ -32,6 +32,8 @@ const DraggableLabel: React.FC<DraggableLabelProps> = (props) => {
|
||||
|
||||
type IconActionProps = {
|
||||
action: string
|
||||
/** Tooltip text, for buttons whose action reads differently from their icon. */
|
||||
title?: string
|
||||
onClick(...args: unknown[]): unknown
|
||||
wdKey?: string
|
||||
classBlockName?: string
|
||||
@@ -62,7 +64,7 @@ class IconAction extends React.Component<IconActionProps> {
|
||||
|
||||
return <button
|
||||
tabIndex={-1}
|
||||
title={this.props.action}
|
||||
title={this.props.title ?? this.props.action}
|
||||
className={`maputnik-layer-list-icon-action ${classAdditions}`}
|
||||
data-wd-key={this.props.wdKey}
|
||||
onClick={this.props.onClick}
|
||||
@@ -111,7 +113,12 @@ export const LayerListItem = React.forwardRef<HTMLLIElement, LayerListItemProps>
|
||||
opacity: isDragging ? 0.5 : 1,
|
||||
};
|
||||
|
||||
const visibilityAction = visibility === "visible" ? "show" : "hide";
|
||||
// The icon and the CSS modifier describe the layer's current visibility,
|
||||
// while the tooltip has to describe what clicking the button does, which is
|
||||
// the opposite of it.
|
||||
const isVisible = visibility === "visible";
|
||||
const visibilityAction = isVisible ? "show" : "hide";
|
||||
const visibilityTitle = isVisible ? "hide" : "show";
|
||||
|
||||
// Cast ref to MutableRefObject since we know from the codebase that's what's always passed
|
||||
const refObject = ref as React.MutableRefObject<HTMLLIElement | null> | null;
|
||||
@@ -155,6 +162,7 @@ export const LayerListItem = React.forwardRef<HTMLLIElement, LayerListItemProps>
|
||||
<IconAction
|
||||
wdKey={"layer-list-item:" + props.layerId + ":toggle-visibility"}
|
||||
action={visibilityAction}
|
||||
title={visibilityTitle}
|
||||
classBlockName="visibility"
|
||||
classBlockModifier={visibilityAction}
|
||||
onClick={_e => onLayerVisibilityToggle!(props.layerIndex)}
|
||||
|
||||
Reference in New Issue
Block a user