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

fix(KeyComboTag): Better rendering on non-Mac operating systems #7025

Merged
merged 10 commits into from
Oct 30, 2024
Merged

fix(KeyComboTag): Better rendering on non-Mac operating systems #7025

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0386e29
fix(KeyComboTag): Better rendering on non-Mac operating systems
jscheiny Oct 24, 2024
f3d75a0
Tests
jscheiny Oct 24, 2024
45fa21e
Add generated changelog entries
svc-changelog Oct 24, 2024
44eb2e7
Add generated changelog entries
svc-changelog Oct 24, 2024
ebd7728
Add generated changelog entries
svc-changelog Oct 24, 2024
d5adb40
Add generated changelog entries
svc-changelog Oct 24, 2024
701f224
Fixes
jscheiny Oct 24, 2024
672aa41
describe -> it
jscheiny Oct 24, 2024
70ebd7f
Fix tests for real
jscheiny Oct 24, 2024
d8d658d
Fix spacing
jscheiny Oct 28, 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
6 changes: 6 additions & 0 deletions packages/core/changelog/@unreleased/pr-7025.v2.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
type: fix
fix:
description: The KeyComboTag component no longer renders modifier key icons on non-Mac
operating systems
links:
- https://github.com/palantir/blueprint/pull/7025
6 changes: 3 additions & 3 deletions packages/core/src/components/hotkeys/hotkeyParser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ export const CONFIG_ALIASES: KeyMap = {
esc: "escape",
escape: "escape",
minus: "-",
mod: isMac() ? "meta" : "ctrl",
mod: isMac(undefined) ? "meta" : "ctrl",
option: "alt",
plus: "+",
return: "enter",
Expand Down Expand Up @@ -232,15 +232,15 @@ export const getKeyCombo = (e: KeyboardEvent): KeyCombo => {
* Unlike the parseKeyCombo method, this method does NOT convert shifted
* action keys. So `"@"` will NOT be converted to `["shift", "2"]`).
*/
export const normalizeKeyCombo = (combo: string, platformOverride?: string): string[] => {
export const normalizeKeyCombo = (combo: string, platformOverride: string | undefined): string[] => {
const keys = combo.replace(/\s/g, "").split("+");
return keys.map(key => {
const keyName = CONFIG_ALIASES[key] != null ? CONFIG_ALIASES[key] : key;
return keyName === "meta" ? (isMac(platformOverride) ? "cmd" : "ctrl") : keyName;
});
};

function isMac(platformOverride?: string) {
export function isMac(platformOverride: string | undefined) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is duplicative with another in the codebase, will be looking into refactoring that in a future change.

// HACKHACK: see https://github.com/palantir/blueprint/issues/5174
// eslint-disable-next-line deprecation/deprecation
const platform = platformOverride ?? (typeof navigator !== "undefined" ? navigator.platform : undefined);
Expand Down
62 changes: 43 additions & 19 deletions packages/core/src/components/hotkeys/keyComboTag.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,20 +33,20 @@ import {
import { AbstractPureComponent, Classes, DISPLAYNAME_PREFIX, type Props } from "../../common";
import { Icon } from "../icon/icon";

import { normalizeKeyCombo } from "./hotkeyParser";
import { isMac, normalizeKeyCombo } from "./hotkeyParser";

const KEY_ICONS: Record<string, { icon: React.JSX.Element; iconTitle: string }> = {
ArrowDown: { icon: <ArrowDown />, iconTitle: "Down key" },
ArrowLeft: { icon: <ArrowLeft />, iconTitle: "Left key" },
ArrowRight: { icon: <ArrowRight />, iconTitle: "Right key" },
ArrowUp: { icon: <ArrowUp />, iconTitle: "Up key" },
alt: { icon: <KeyOption />, iconTitle: "Alt/Option key" },
cmd: { icon: <KeyCommand />, iconTitle: "Command key" },
ctrl: { icon: <KeyControl />, iconTitle: "Control key" },
const KEY_ICONS: Record<string, { icon: React.JSX.Element; iconTitle: string; isMacOnly?: boolean }> = {
alt: { icon: <KeyOption />, iconTitle: "Alt/Option key", isMacOnly: true },
arrowdown: { icon: <ArrowDown />, iconTitle: "Down key" },
arrowleft: { icon: <ArrowLeft />, iconTitle: "Left key" },
arrowright: { icon: <ArrowRight />, iconTitle: "Right key" },
arrowup: { icon: <ArrowUp />, iconTitle: "Up key" },
cmd: { icon: <KeyCommand />, iconTitle: "Command key", isMacOnly: true },
ctrl: { icon: <KeyControl />, iconTitle: "Control key", isMacOnly: true },
delete: { icon: <KeyDelete />, iconTitle: "Delete key" },
enter: { icon: <KeyEnter />, iconTitle: "Enter key" },
meta: { icon: <KeyCommand />, iconTitle: "Command key" },
shift: { icon: <KeyShift />, iconTitle: "Shift key" },
meta: { icon: <KeyCommand />, iconTitle: "Command key", isMacOnly: true },
shift: { icon: <KeyShift />, iconTitle: "Shift key", isMacOnly: true },
};

/** Reverse table of some CONFIG_ALIASES fields, for display by KeyComboTag */
Expand All @@ -71,20 +71,30 @@ export interface KeyComboTagProps extends Props {
minimal?: boolean;
}

export class KeyComboTag extends AbstractPureComponent<KeyComboTagProps> {
interface KeyComboTagInternalProps extends KeyComboTagProps {
/** Override the oeprating system rendering for internal testing purposes */
platformOverride?: string;
}

export class KeyComboTagInternal extends AbstractPureComponent<KeyComboTagInternalProps> {
public static displayName = `${DISPLAYNAME_PREFIX}.KeyComboTag`;

public render() {
const { className, combo, minimal } = this.props;
const keys = normalizeKeyCombo(combo)
const { className, combo, minimal, platformOverride } = this.props;
const normalizedKeys = normalizeKeyCombo(combo, platformOverride);
const keys = normalizedKeys
.map(key => (key.length === 1 ? key.toUpperCase() : key))
.map(minimal ? this.renderMinimalKey : this.renderKey);
.map((key, index) =>
minimal
? this.renderMinimalKey(key, index, index === normalizedKeys.length - 1)
: this.renderKey(key, index),
);
return <span className={classNames(Classes.KEY_COMBO, className)}>{keys}</span>;
}

private renderKey = (key: string, index: number) => {
const keyString = DISPLAY_ALIASES[key] ?? key;
const icon = KEY_ICONS[key];
const icon = this.getKeyIcon(key);
const reactKey = `key-${index}`;
return (
<kbd className={classNames(Classes.KEY, { [Classes.MODIFIER_KEY]: icon != null })} key={reactKey}>
Expand All @@ -94,8 +104,22 @@ export class KeyComboTag extends AbstractPureComponent<KeyComboTagProps> {
);
};

private renderMinimalKey = (key: string, index: number) => {
const icon = KEY_ICONS[key];
return icon == null ? key : <Icon icon={icon.icon} title={icon.iconTitle} key={`key-${index}`} />;
private renderMinimalKey = (key: string, index: number, isLastKey: boolean) => {
const icon = this.getKeyIcon(key);
if (icon == null) {
return isLastKey ? key : <React.Fragment key={`key-${index}`}>{key}&nbsp;+&nbsp;</React.Fragment>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add the "+" between tokens regardless of icon/platform? IMO, the minimal version looks a bit better with it on mac. I also notice there's a bit of extra space after the first token if the next token happens to be another icon.

Screenshot 2024-10-25 at 16 43 28@2x

Screenshot 2024-10-25 at 16 50 48@2x

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following that, I wonder if we could insert the plus signs an alternative way, perhaps either via a CSS ::before pseudo-element or by inserting them after construction of the array, e.g.

const normalizedKeys = normalizeKeyCombo(combo, platformOverride);
const upperKeys = normalizedKeys.map(key => (key.length === 1 ? key.toUpperCase() : key));
const keys = upperKeys.map(this.renderKey);
const minimalKeys = upperKeys
    .map(this.renderMinimalKey)
    .flatMap((value, index, array) =>
        array.length - 1 !== index ? [value, <span key={index}>+</span>] : value,
    );
return <span className={classNames(Classes.KEY_COMBO, className)}>{minimal ? minimalKeys : keys}</span>;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to mirror the ways these different platforms display key combos. Namely on Windows/Linux, will show the plus signs and on Macs they will not. Agreed on the extra space, will fix that.

In terms of putting in the plus signs, I attempted to use the pseudo elements earlier but getting the styles to work on those turned out to be unwieldy. Its not clear to me that the code above is particularly better than what we have currently.

}
return <Icon icon={icon.icon} title={icon.iconTitle} key={`key-${index}`} />;
};

private getKeyIcon(key: string) {
const { platformOverride } = this.props;
const icon = KEY_ICONS[key];
if (icon?.isMacOnly && !isMac(platformOverride)) {
return undefined;
}
return icon;
}
}

export const KeyComboTag: React.ComponentType<KeyComboTagProps> = KeyComboTagInternal;
17 changes: 15 additions & 2 deletions packages/core/test/hotkeys/keyComboTagTests.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,24 @@ import { render, screen } from "@testing-library/react";
import { expect } from "chai";
import * as React from "react";

import { KeyComboTag } from "../../src/components/hotkeys";
import { KeyComboTagInternal } from "../../src/components/hotkeys/keyComboTag";

describe("KeyCombo", () => {
it("renders key combo", () => {
render(<KeyComboTag combo="cmd+C" />);
render(<KeyComboTagInternal combo="cmd+C" platformOverride="Mac" />);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we using this platformOverride flag solely for testing purposes? If so, that feels a slight bit icky to me. Is there a way we could potentially mock the return of the isMac function instead? This would potentially save us from having to wire this override flag in multiple places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed its ugly. However, I took a look around and wasn't able to find a good way to mock isMac, but let me know if I missed something here. I did make the argument required on these functions so that we wouldn't forget to pass it and miss a spot.

expect(screen.getByText("C")).not.to.be.undefined;
});

it("should render minimal key combos on Mac using icons", () => {
render(<KeyComboTagInternal combo="mod+C" minimal={true} platformOverride="Mac" />);
expect(() => screen.getByText("cmd + C", { exact: false })).to.throw;
});

it("should render minimal key combos on non-Macs using text", () => {
render(<KeyComboTagInternal combo="mod+C" minimal={true} platformOverride="Win32" />);
const text = screen.getByText("ctrl + C", { exact: false }).innerText;
expect(text).to.contain("ctrl");
expect(text).to.contain("+");
expect(text).to.contain("C");
});
});
6 changes: 6 additions & 0 deletions packages/docs-app/changelog/@unreleased/pr-7025.v2.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
type: fix
fix:
description: The useHotkeys documentation now shows minimal and non-minimal states
for the KeyComboTag component.
links:
- https://github.com/palantir/blueprint/pull/7025
1 change: 1 addition & 0 deletions packages/docs-app/src/examples/core-examples/hotkeyTesterExample.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ export class HotkeyTesterExample extends React.PureComponent<ExampleProps, Hotke
return (
<>
<KeyComboTag combo={combo} />
<KeyComboTag combo={combo} minimal={true} />
<Code>{combo}</Code>
</>
);
Expand Down