Skip to content

Commit

Permalink
Connect: Enable keyboard shortcuts configuration (#22194)
Browse files Browse the repository at this point in the history 
* Use event.code for non A-z keys

Without this fix, shortcuts like "Shift+1" can't work, because the reported character is "!".

* Add Zod schema to validate keyboard shortcuts

* Render better formatted error notification, add a link to docs

* Notify about duplicated accelerators

* Improve the error message for re-parsing failure (it can happen when a default value does not pass validation)

* A bunch of renames

* Allow spaces in accelerators

* Rename `isContentAnObject`

* Allow `Notification` to render links and lists

* Split by optional whitespace and "+"

* Allow lowercase letters

* Require a modifier for non-function keys

* Move VALID_SHORTCUT_MESSAGE to `initUi`

* Always return from `getDuplicateAccelerators`

* Use 'Cmd' and 'Ctrl' for mac

* Add comments

* Fix notification not rendering content

* Add more comments, rename `getKeyCode` to `getKeyName`

* Fix incorrect size of <Link> text

* Remove "expected" prefix

* Revert `typeof content === 'object'` in `Notification`

* Remove a comment about disabled keys in `ConfigService`, add a note about `keymap.` prefix

* Improve `getKeyName` comment

* Extract an inline object to a variable in `getKeyName`

* Fix notification list padding

* Change text for doc link & description for config error

* Improve comment for `getKeyName`

* Remove special formatting for list === 1 in `Notification`

* Print valid modifiers

* Call `getKeyboardShortcutSchema()` once

* Collect issues from all validations, run `invalidModifiers` through a set

* Change error message for `missingModifierIssue`

* Convert duplicates warning to error

* Define ALLOWED_KEY_CODES in a more concise way

* Support both `IntlBackslash` and `Backquote`

* Restore modifiers for mac to full spelling, sort them in order recommended by platform guidelines

* Fix old comment about the shortcuts order

---------

Co-authored-by: Rafał Cieślak <[email protected]>
  • Loading branch information
@gzdunek @ravicious
gzdunek and ravicious authored Mar 3, 2023
1 parent 717c4fa commit 4830ea1
Show file tree
Hide file tree
Showing 11 changed files with 666 additions and 88 deletions.
112 changes: 110 additions & 2 deletions web/packages/shared/components/Notification/Notification.story.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,13 @@ export default {

export const Notifications = () => {
return (
<Flex gap={8}>
<div
css={`
display: grid;
grid-gap: ${props => props.theme.space[8]}px;
grid-template-columns: auto auto auto;
`}
>
<Flex flexDirection="column" gap={4}>
<Notification
item={{
Expand Down Expand Up @@ -149,7 +155,109 @@ export const Notifications = () => {
isAutoRemovable={false}
/>
</Flex>
</Flex>

<Flex flexDirection="column" gap={4}>
<Notification
item={{
id: crypto.randomUUID(),
severity: 'info',
content: {
title: 'Info with link',
description: loremIpsum,
link: {
href: 'https://goteleport.com',
text: 'goteleport.com',
},
},
}}
Icon={Info}
getColor={theme => theme.colors.info}
onRemove={() => {}}
isAutoRemovable={false}
/>
<Notification
item={{
id: crypto.randomUUID(),
severity: 'warn',
content: {
title: 'Warning with link',
description: loremIpsum,
link: {
href: 'https://goteleport.com',
text: 'goteleport.com',
},
},
}}
Icon={Warning}
getColor={theme => theme.colors.warning}
onRemove={() => {}}
isAutoRemovable={false}
/>
<Notification
item={{
id: crypto.randomUUID(),
severity: 'error',
content: {
title: 'Error with link',
description: loremIpsum,
link: {
href: 'https://goteleport.com',
text: 'goteleport.com',
},
},
}}
Icon={Warning}
getColor={theme => theme.colors.danger}
onRemove={() => {}}
isAutoRemovable={false}
/>
</Flex>

<Flex flexDirection="column" gap={4}>
<Notification
item={{
id: crypto.randomUUID(),
severity: 'info',
content: {
title: 'Info with list',
list: [loremIpsum, loremIpsum],
},
}}
Icon={Info}
getColor={theme => theme.colors.info}
onRemove={() => {}}
isAutoRemovable={false}
/>
<Notification
item={{
id: crypto.randomUUID(),
severity: 'warn',
content: {
title: 'Warning with list',
list: [loremIpsum, loremIpsum],
},
}}
Icon={Warning}
getColor={theme => theme.colors.warning}
onRemove={() => {}}
isAutoRemovable={false}
/>
<Notification
item={{
id: crypto.randomUUID(),
severity: 'error',
content: {
title: 'Error with list',
list: [loremIpsum, loremIpsum],
},
}}
Icon={Warning}
getColor={theme => theme.colors.danger}
onRemove={() => {}}
isAutoRemovable={false}
/>
</Flex>
</div>
);
};

Expand Down
39 changes: 38 additions & 1 deletion web/packages/shared/components/Notification/Notification.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,20 @@ limitations under the License.

import React, { useEffect, useRef, useState } from 'react';
import styled, { css, useTheme } from 'styled-components';
import { ButtonIcon, Flex, Text } from 'design';
import { ButtonIcon, Flex, Link, Text } from 'design';
import { Close } from 'design/Icon';

import type { NotificationItem, NotificationItemContent } from './types';

interface NotificationProps {
item: NotificationItem;

onRemove(): void;

Icon: React.ElementType;

getColor(theme): string;

isAutoRemovable: boolean;
autoRemoveDurationMs?: number;
// Workaround until `styled` gets types.
Expand Down Expand Up @@ -171,13 +175,46 @@ function getRenderedContent(
color="text.secondary"
css={longerTextCss}
>
{content.list && <List items={content.list} />}
{content.description}
{content.link && (
<Link
css={`
display: block;
`}
href={content.link.href}
target="_blank"
>
{content.link.text}
</Link>
)}
</Text>
</Flex>
);
}
}

function List(props: { items: string[] }) {
return (
<ul
// Ideally we'd align the bullet point to the left without using list-style-position: inside
// (because it looks bad when the list item spans multiple lines).
//
// However, it seems impossible to use padding-inline-start for that because the result looks
// different on Retina vs non-Retina screens, the bullet point looks cut off on the latter if
// padding-inline-start is set to 1em. So instead we just set it to 2em.
css={`
margin: 0;
padding-inline-start: 2em;
`}
>
{props.items.map((item, index) => (
<li key={index}>{item}</li>
))}
</ul>
);
}

const textCss = css`
line-height: 20px;
overflow-wrap: break-word;
Expand Down
14 changes: 11 additions & 3 deletions web/packages/shared/components/Notification/types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,14 @@ export interface NotificationItem {
id: string;
}

export type NotificationItemContent =
| string
| { title: string; description: string };
export type NotificationItemContent = string | NotificationItemObjectContent;

export type NotificationItemObjectContent = {
title: string;
list?: string[];
description?: string;
link?: {
href: string;
text: string;
};
};
97 changes: 32 additions & 65 deletions web/packages/teleterm/src/services/config/configService.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,70 +20,45 @@ import { FileStorage } from 'teleterm/services/fileStorage';
import { Platform } from 'teleterm/mainProcess/types';

import { createConfigStore } from './configStore';
import { getKeyboardShortcutSchema } from './getKeyboardShortcutSchema';

const createAppConfigSchema = (platform: Platform) => {
const defaultKeymap = getDefaultKeymap(platform);
const defaultTerminalFont = getDefaultTerminalFont(platform);

// Important: all keys except 'usageReporting.enabled' are currently not
// configurable by the user. Before we let the user configure them,
// we need to set up some actual validation, so that for example
// arbitrary CSS cannot be injected into the app through font settings.
//
// However, we want them to be in the config schema, so we included
// them here, but we do not read their value from the stored config.
const keyboardShortcutSchema = getKeyboardShortcutSchema(platform);

// `keymap.` prefix is used in `initUi.ts` in a predicate function.
return z.object({
'usageReporting.enabled': z.boolean().default(false),
'keymap.tab1': omitStoredConfigValue(
z.string().default(defaultKeymap['tab1'])
),
'keymap.tab2': omitStoredConfigValue(
z.string().default(defaultKeymap['tab2'])
),
'keymap.tab3': omitStoredConfigValue(
z.string().default(defaultKeymap['tab3'])
),
'keymap.tab4': omitStoredConfigValue(
z.string().default(defaultKeymap['tab4'])
),
'keymap.tab5': omitStoredConfigValue(
z.string().default(defaultKeymap['tab5'])
),
'keymap.tab6': omitStoredConfigValue(
z.string().default(defaultKeymap['tab6'])
),
'keymap.tab7': omitStoredConfigValue(
z.string().default(defaultKeymap['tab7'])
),
'keymap.tab8': omitStoredConfigValue(
z.string().default(defaultKeymap['tab8'])
),
'keymap.tab9': omitStoredConfigValue(
z.string().default(defaultKeymap['tab9'])
),
'keymap.closeTab': omitStoredConfigValue(
z.string().default(defaultKeymap['closeTab'])
),
'keymap.newTab': omitStoredConfigValue(
z.string().default(defaultKeymap['newTab'])
),
'keymap.previousTab': omitStoredConfigValue(
z.string().default(defaultKeymap['previousTab'])
),
'keymap.nextTab': omitStoredConfigValue(
z.string().default(defaultKeymap['nextTab'])
),
'keymap.openConnections': omitStoredConfigValue(
z.string().default(defaultKeymap['openConnections'])
),
'keymap.openClusters': omitStoredConfigValue(
z.string().default(defaultKeymap['openClusters'])
),
'keymap.openProfiles': omitStoredConfigValue(
z.string().default(defaultKeymap['openProfiles'])
),
'keymap.openQuickInput': omitStoredConfigValue(
z.string().default(defaultKeymap['openQuickInput'])
'keymap.tab1': keyboardShortcutSchema.default(defaultKeymap['tab1']),
'keymap.tab2': keyboardShortcutSchema.default(defaultKeymap['tab2']),
'keymap.tab3': keyboardShortcutSchema.default(defaultKeymap['tab3']),
'keymap.tab4': keyboardShortcutSchema.default(defaultKeymap['tab4']),
'keymap.tab5': keyboardShortcutSchema.default(defaultKeymap['tab5']),
'keymap.tab6': keyboardShortcutSchema.default(defaultKeymap['tab6']),
'keymap.tab7': keyboardShortcutSchema.default(defaultKeymap['tab7']),
'keymap.tab8': keyboardShortcutSchema.default(defaultKeymap['tab8']),
'keymap.tab9': keyboardShortcutSchema.default(defaultKeymap['tab9']),
'keymap.closeTab': keyboardShortcutSchema.default(
defaultKeymap['closeTab']
),
'keymap.newTab': keyboardShortcutSchema.default(defaultKeymap['newTab']),
'keymap.previousTab': keyboardShortcutSchema.default(
defaultKeymap['previousTab']
),
'keymap.nextTab': keyboardShortcutSchema.default(defaultKeymap['nextTab']),
'keymap.openConnections': keyboardShortcutSchema.default(
defaultKeymap['openConnections']
),
'keymap.openClusters': keyboardShortcutSchema.default(
defaultKeymap['openClusters']
),
'keymap.openProfiles': keyboardShortcutSchema.default(
defaultKeymap['openProfiles']
),
'keymap.openQuickInput': keyboardShortcutSchema.default(
defaultKeymap['openQuickInput']
),
/**
* This value can be provided by the user and is unsanitized. This means that it cannot be directly interpolated
Expand All @@ -96,16 +71,8 @@ const createAppConfigSchema = (platform: Platform) => {
});
};

const omitStoredConfigValue = <T>(schema: z.ZodType<T>) =>
z.preprocess(() => undefined, schema);

export type AppConfig = z.infer<ReturnType<typeof createAppConfigSchema>>;

/**
* Modifier keys must be defined in the following order:
* Command-Control-Option-Shift for macOS
* Ctrl-Alt-Shift for other platforms
*/
export type KeyboardShortcutAction =
| 'tab1'
| 'tab2'
Expand Down
6 changes: 4 additions & 2 deletions web/packages/teleterm/src/services/config/configStore.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,10 @@ export function createConfigStore<
});
const reParsed = parse(withoutInvalidKeys);
if (reParsed.success === false) {
// it should not occur after removing invalid keys, but just in case
throw new Error('Re-parsing config file failed', reParsed.error.cause);
// it can happen when a default value does not pass validation
throw new Error(
`Re-parsing config file failed \n${reParsed.error.message}`
);
}
return {
storedConfig: withoutInvalidKeys,
Expand Down
Loading

0 comments on commit 4830ea1

Please sign in to comment.