Added Keyboard shortcuts modal #6112

akshayasalvi · 2021-10-29T12:45:06Z

Details

Added help dialog for keyboard shortcuts list

Fixed Issues

$ #5588

Tests

Checked keyboard shortcut with Desktop and Web
Checked from Windows Browser for Ctrl + ? shortcut.
Compared all the current keyboard shortcuts.

QA Steps

On the sidebar click Cmd + ?
It should open a dialog with the list of keyboard shortcuts.
It should cover all the current keyboard shortcuts.

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

akshayasalvi · 2021-10-29T12:49:53Z

@roryabraham Need help with three things.

Our current modal only supports full screen for the desktop

I feel should modify this to a centred modal. Can you help me understand a better approach here?
I've tried overriding modalContainerStyles it doesn't work. I can see we're using getModalStyles. Should I add a new style type in the modal switch block?

I have used a combination of Views to create a Table grid. I didn't find any other component that can be used to create a table. Just left with the borderRadius.
Need to verify spanish translations

Also @shawnborton Can you confirm the colors for the styling and if the borderRadius is required for the table?

shawnborton · 2021-11-01T18:26:38Z

Can we use a smaller modal style for this? I don't think it needs to take up the full screen. I'm also curious to see what mobile looks like?

I think a border radius on the table would look nice. I don't think we need gray backgrounds for the left column cells either, perhaps we can just give those cells a right border.

shawnborton · 2021-11-01T18:26:55Z

It also looks like the table is not using the correct font family.

akshayasalvi · 2021-11-03T14:46:58Z

@shawnborton I've updated screenshots in GH Body.

I'm also curious to see what mobile looks like?
I haven't set any click trigger. Only keyboard shortcut trigger. Can you suggest what would be the trigger on the mobile?

It also looks like the table is not using the correct font family.
I didn't change/set anything specific as such previously. Now I've set fontFamily: GTA.

@roryabraham Can you please review this?

roryabraham

This did not pass testing. Pressing CMD + ? opens a help menu on desktop:

src/languages/en.js

rosegrech · 2021-11-03T21:23:34Z

@roryabraham we use the waiting for copy label to get copy reviewed which I applied but it did not ping anyone. Do you know if that label does not work in these types of GHs?

roryabraham · 2021-11-03T21:41:19Z

Sorry @rosegrech, I should've remembered that. It didn't work because this is a pull request, not an issue.

rosegrech · 2021-11-04T14:33:05Z

no worries @roryabraham good for me to know that.

Amazing catch, I see what you mean. It looks like the shortcuts are all about chat yeah?
What about something for vague like:

Save time with these handy keyboard shortcuts:

akshayasalvi · 2021-11-14T00:49:36Z

Resolved conflicts and pushed. @roryabraham Any other changes required for this one?

src/libs/KeyboardShortcut/index.js

src/styles/styles.js

src/components/KeyboardShortcutsModal.js

src/libs/Navigation/AppNavigator/AuthScreens.js

akshayasalvi · 2021-11-19T13:35:01Z

@roryabraham PR updated. But I was wondering if, after the new KEYBOARD_SHORTCUTS map, we should modify the subscribe function with just two params shortcutConfig, callback. We also move the captureOnInputs to the config itself.

and then the function calls be like ``

this.unsubscribeShortcutModal = KeyboardShortcut.subscribe(CONST.KEYBOARD_SHORTCUTS.SHORTCUT_MODAL.shortcutKey, () => {
    this.showKeyboardShortcutModal();
});

roryabraham

Overall great progress! I love the work that we've done here to create an extensible framework for keyboard shortcuts in our app, and I think we are now very close to a final version.

I also want to thank you for your patience with me as we've iteratively improved this design in several rounds of reviews. Almost there, let's get this over the finish line!

src/components/KeyboardShortcutsModal.js

roryabraham · 2021-11-22T21:10:07Z

src/components/KeyboardShortcutsModal.js

+                isVisible={this.state.isOpen}
+                type={modalType}
+                containerStyle={styles.keyboardShortcutModalContainer}
+                onClose={() => this.hideKeyboardShortcutModal()}


Suggested change

onClose={() => this.hideKeyboardShortcutModal()}

onClose={this.hideKeyboardShortcutModal}

roryabraham · 2021-11-22T21:10:25Z

src/components/KeyboardShortcutsModal.js

+                containerStyle={styles.keyboardShortcutModalContainer}
+                onClose={() => this.hideKeyboardShortcutModal()}
+            >
+                <HeaderWithCloseButton title={this.props.translate('keyboardShortcutModal.title')} onCloseButtonPress={() => this.hideKeyboardShortcutModal()} />


Suggested change

<HeaderWithCloseButton title={this.props.translate('keyboardShortcutModal.title')} onCloseButtonPress={() => this.hideKeyboardShortcutModal()} />

<HeaderWithCloseButton title={this.props.translate('keyboardShortcutModal.title')} onCloseButtonPress={this.hideKeyboardShortcutModal} />

roryabraham · 2021-11-22T21:31:23Z

src/libs/KeyboardShortcut/index.js

+function getKeyboardShortcuts() {
+    const shortcuts = [];
+    _.each(keyboardShortcutMap, (descriptionKey, key) => {
+        shortcuts.push({key, descriptionKey});


Just noticing a subtle inconsistency here in that in CONST.js we define keyboard shortcuts as having the following shape:

{ shortcutKey: String, descriptionKey: String, modifiers: Array<String>, }

But here we're defining them with this a slightly different shape:

{ key: String, // this is really a string with modifiers + the key, descriptionKey: String }

So I find this similar but separate representation a bit confusing, and I think we could make this a bit clearer by having keyboardShortcutMap store a representation of keyboard shortcuts with the following shape:

{ // The key that is pressed to initiate some action shortcutKey: String, // Modifier keys that also must be pressed with the shortcut key to initiate this action modifiers: Array<String>, // The human-readable representation of the shortcut with its modifiers (i.e: `CMD + SHIFT + K`) displayName: String, // A translation key for the full description of the action initiated by this keyboard shortcut descriptionKey: String, }

akshayasalvi · 2021-11-23T20:56:46Z

@roryabraham PR updated

roryabraham

This PR is looking great! Thanks for making all those changes @akshayasalvi! Really happy with how this ended up.

OSBotify · 2021-11-24T06:07:13Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

parasharrajat · 2021-11-24T07:53:38Z

Wow, This PR has whoopping 42 commits.

roryabraham · 2021-11-24T18:37:21Z

@akshayasalvi Just FYI, this PR caused a crash on iOS/Android because we renamed methods on KeyboardShortcut/index.js but didn't rename them on KeyboardShortcut/index.native.js. @parasharrajat Put up a fix in this PR, but it's something we should have caught before merging.

akshayasalvi · 2021-11-25T03:39:49Z

Noted @roryabraham. Sorry about that. Thanks for fixing @parasharrajat

OSBotify · 2021-11-29T19:57:31Z

🚀 Deployed to staging by @roryabraham in version: 1.1.16-11 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

ogumen · 2021-11-30T14:07:35Z

The accessibility issues found in this PR:

The X (Close) button fails minimum color contrast requirements of 3.00:1 against the background. The light grey button (#C6C9CA) against white background (#FFFFFF) has a color contrast ratio of 1.66:1 - failure of WCAG SC 1.4.11, similar to [med] Color Contrast: Many Pages: Grey icons against white background fail contrast requirements #5506
The visual dialog heading "Keyboard Shortcuts" is not implemented semantically as a heading and not announced as heading by screen reader - failure of WCAG SC 1.3.1, similar to [low] JAWS+Chrome: Many Pages: Visual headings are not announced as such #5458
The shortcuts dialog has no semantic title provided, no label is announced by screen reader on dialog opening - failure of WCAG SC 1.3.1
Non-interactive dialog heading is focusable with Tab key - failure of WCAG SC 2.4.3

Non-interactive.Keyboard.Shortcuts.heading.is.focused.with.Tab.key.mp4

The question mark in "Ctrl+?" command is not announced by screen reader - failure of WCAG SC 1.3.2. This issue may be fixed by hiding the visible command from the screen reader using aria-hidden attribute and adding an off-screen text "Control plus question mark" to provide this information to screen reader users.

The.question.mark.is.no.announced.as.part.of.keyboard.command.mp4

OSBotify · 2021-12-06T23:26:13Z

🚀 Deployed to production by @roryabraham in version: 1.1.17-7 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

eVoloshchak · 2023-06-02T16:38:40Z

src/libs/KeyboardShortcut/index.js

+ * @returns {Array}
+ */
+function getKeyboardShortcuts() {
+    return _.values(keyboardShortcutMap);


This has introduced a small regression in #18542.
In short, the order of keyboard shortcuts wasn't consistent across browsers, we solved this by using .sort()

akshayasalvi added 11 commits October 29, 2021 16:17

Basic component with props added

baf320a

Converted component to class component

fdf81af

Added mapper function to the keyboardshortcut lib

e00816c

Translation files

28855fb

Removed modal props from ScreenWrapper

cb54938

Modified the shortcut trigger logic to modal

a6c28bd

Added keys for existing shortcuts

6bad775

Styling updates for container

290acb8

Key handling for cmd

f31f5ed

Moved styling to style.js

2b6e319

Changed border styling

32622ab

akshayasalvi requested a review from a team as a code owner October 29, 2021 12:45

MelvinBot requested review from roryabraham and removed request for a team October 29, 2021 12:45

Added spanish translations

2e0d4c7

akshayasalvi added 4 commits November 3, 2021 19:17

Added styling options for modal

2096196

Added borderRadius to the table

18fbadf

Fixed border styling for the table

640687d

Removed height hardcoding

c18e2a4

roryabraham assigned akshayasalvi Nov 3, 2021

roryabraham requested changes Nov 3, 2021

View reviewed changes

src/languages/en.js Outdated Show resolved Hide resolved

rosegrech added the Waiting for copy User facing verbiage needs polishing label Nov 3, 2021

roryabraham requested changes Nov 15, 2021

View reviewed changes

akshayasalvi added 5 commits November 19, 2021 18:18

Changed logic for modifiers

4c29fc0

Fixed typo for KeyboardShortcut

7de6c54

Split toggle func to hide and show

e5b034d

Removed hardcoding of the shortcuts to CONST

3cbd1bf

Pick all shortcut configs from CONST

fc03898

akshayasalvi added 2 commits November 19, 2021 19:15

Added enter configuration for keyboard shortcuts

eb6ef1c

Fixed typo for Button

e2e456c

roryabraham requested changes Nov 22, 2021

View reviewed changes

akshayasalvi added 3 commits November 24, 2021 02:25

Added jsdocs and changed function calls

f09b027

Unified representation of shortcut object

2eeed69

Fixed typos in shortcuts

06c77ce

roryabraham approved these changes Nov 24, 2021

View reviewed changes

roryabraham merged commit 36558e0 into Expensify:main Nov 24, 2021

parasharrajat mentioned this pull request Nov 24, 2021

[NO QA] fix: crash #6462

Merged

4 tasks

OSBotify mentioned this pull request Nov 29, 2021

Deploy Checklist: New Expensify 2021-11-29 #6505

Closed

69 tasks

This was referenced Dec 6, 2021

[$250] Add a Welcome step to the set password sign in flow #5079

Closed

[HOLD for payment 2021-12-13] $500 New Feature - Add a modal with the existing keyboard shortcuts - issue by @akshayasalvi #5588

Closed

eVoloshchak reviewed Jun 2, 2023

View reviewed changes

eVoloshchak mentioned this pull request Jun 2, 2023

[HOLD for payment 2023-06-02] [$1000] The ordering of the last two keyboard shortcuts is not consistent across different browsers #18542

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Keyboard shortcuts modal #6112

Added Keyboard shortcuts modal #6112

akshayasalvi commented Oct 29, 2021 •

edited

Loading

akshayasalvi commented Oct 29, 2021 •

edited

Loading

shawnborton commented Nov 1, 2021

shawnborton commented Nov 1, 2021

akshayasalvi commented Nov 3, 2021

roryabraham left a comment

rosegrech commented Nov 3, 2021

roryabraham commented Nov 3, 2021

rosegrech commented Nov 4, 2021

akshayasalvi commented Nov 14, 2021

akshayasalvi commented Nov 19, 2021

roryabraham left a comment

roryabraham Nov 22, 2021

roryabraham Nov 22, 2021

roryabraham Nov 22, 2021

akshayasalvi commented Nov 23, 2021

roryabraham left a comment

OSBotify commented Nov 24, 2021

parasharrajat commented Nov 24, 2021

roryabraham commented Nov 24, 2021

akshayasalvi commented Nov 25, 2021 •

edited

Loading

OSBotify commented Nov 29, 2021

ogumen commented Nov 30, 2021

OSBotify commented Dec 6, 2021

eVoloshchak Jun 2, 2023

	onClose={() => this.hideKeyboardShortcutModal()}
	onClose={this.hideKeyboardShortcutModal}

	<HeaderWithCloseButton title={this.props.translate('keyboardShortcutModal.title')} onCloseButtonPress={() => this.hideKeyboardShortcutModal()} />
	<HeaderWithCloseButton title={this.props.translate('keyboardShortcutModal.title')} onCloseButtonPress={this.hideKeyboardShortcutModal} />

Added Keyboard shortcuts modal #6112

Added Keyboard shortcuts modal #6112

Conversation

akshayasalvi commented Oct 29, 2021 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

akshayasalvi commented Oct 29, 2021 • edited Loading

shawnborton commented Nov 1, 2021

shawnborton commented Nov 1, 2021

akshayasalvi commented Nov 3, 2021

roryabraham left a comment

Choose a reason for hiding this comment

rosegrech commented Nov 3, 2021

roryabraham commented Nov 3, 2021

rosegrech commented Nov 4, 2021

akshayasalvi commented Nov 14, 2021

akshayasalvi commented Nov 19, 2021

roryabraham left a comment

Choose a reason for hiding this comment

roryabraham Nov 22, 2021

Choose a reason for hiding this comment

roryabraham Nov 22, 2021

Choose a reason for hiding this comment

roryabraham Nov 22, 2021

Choose a reason for hiding this comment

akshayasalvi commented Nov 23, 2021

roryabraham left a comment

Choose a reason for hiding this comment

OSBotify commented Nov 24, 2021

parasharrajat commented Nov 24, 2021

roryabraham commented Nov 24, 2021

akshayasalvi commented Nov 25, 2021 • edited Loading

OSBotify commented Nov 29, 2021

ogumen commented Nov 30, 2021

OSBotify commented Dec 6, 2021

eVoloshchak Jun 2, 2023

Choose a reason for hiding this comment

akshayasalvi commented Oct 29, 2021 •

edited

Loading

akshayasalvi commented Oct 29, 2021 •

edited

Loading

akshayasalvi commented Nov 25, 2021 •

edited

Loading