fix(KeyComboTag): Better rendering on non-Mac operating systems#7025
Conversation
Generate changelog in
|
fix(KeyComboTag): Better rendering on non-Mac operating systemsBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
| }; | ||
|
|
||
| function isMac(platformOverride?: string) { | ||
| export function isMac(platformOverride: string | undefined) { |
There was a problem hiding this comment.
This function is duplicative with another in the codebase, will be looking into refactoring that in a future change.
Add generated changelog entriesBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
FixesBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
describe -> itBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Fix tests for realBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
| describe("KeyCombo", () => { | ||
| it("renders key combo", () => { | ||
| render(<KeyComboTag combo="cmd+C" />); | ||
| render(<KeyComboTagInternal combo="cmd+C" platformOverride="Mac" />); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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} + </React.Fragment>; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>;There was a problem hiding this comment.
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.
4 participants
Fixes #6211
Checklist
Changes proposed in this pull request:
In this PR we update the
KeyComboTagcomponent to render properly in non-Mac environments. Namely, all modifier keys no longer show their icons (which are mac specific). In addition the minimal state has been updated to show a more windows friendly UI. Finally, the arrow keys were broken and not showing their icons, they do now.Reviewers should focus on:
I implemented a
KeyComboTagInternalcomponent for testing purposes. What do we think of this pattern?Screenshot
Previously, this is how the
mod+shift+xkey combo looked like on all platforms. This is still how it looks on Mac. (Minimal state on bottom)This is how it looks non-Mac platforms now:
