Add Cmd+Shift+F as tidy code shortcut and prevent browser default behaviour #1732

vulongphan · 2021-01-08T09:44:24Z

I have verified that this pull request:

has no linting errors (npm run lint)
is from a uniquely-named feature branch and has been rebased on top of the latest develop branch. (If I was asked to make more changes, I have made sure to rebase onto develop then too)
is descriptively named and links to an issue number, i.e. Fixes #123

…viour

vulongphan · 2021-01-08T09:50:11Z

@catarak what I did was to detect key short cut of Cmd+Shift+F to call tidyCode() and to prevent browser default behaviour (Full Screen Mode in Chrome on Mac). Could you please have a look and tell me if I have to change anything?

catarak · 2021-01-20T22:32:34Z

client/modules/IDE/components/Editor.jsx

    });

    this._cm.on('keydown', (_cm, e) => {
-      // 9 === Tab
-      if (e.keyCode === 9 && e.shiftKey) {
+      this.handleKey(this.map, e);


I'm not sure how using this function this.handleKey is helping in this scenario? Just from reading this it seems like it's adding some intermediate code that is unnecessary, but please explain if I'm wrong!

The reason why I use this function is because to detect shortcut Cmd+Shift+F the following conditional statement would not work

if (e.keyCode === 91 && e.keyCode === 16 && e.keyCode === 70){ }

I guess the reason is because the browser cannot detect multiple key press. To solve this, I used an object called this.map to store the keydown state of the keyboard. For example, if the shortcut Cmd+Shift+F is pressed, this.map will be

{91: true, 16: true, 70: true}

and when we release the shortcut, this.map will be

{91: false, 16: false, 70: false}

I wonder if there is a better way to get around this.

there is! if you look at the documentation for a KeyboardEvent, you'll see that it contains metaKey, which will be true if Command is pressed, ctrlKey, which will be true if Control is pressed, and shiftKey, which will be true if Shift is pressed.

catarak · 2021-01-20T22:35:46Z

client/modules/IDE/components/Editor.jsx

-      // 9 === Tab
-      if (e.keyCode === 9 && e.shiftKey) {
+      this.handleKey(this.map, e);
+      // 91 === Cmd


There's a variable that's imported to this file called metaKey which will be set to 'Cmd' if the user is on Mac, and 'Ctrl' if the user is on Windows/Linux. I think you should use this variable rather than just 'Cmd' since 'Cmd' doesn't exist on Windows/Linux!

Yes, I will incorporate the use of metaKey in my changes!

catarak · 2021-01-20T22:36:15Z

client/modules/IDE/components/Editor.jsx

+
+// delete CodeMirror.keyMap.sublime['Shift-Cmd-F'];
+
+console.log(CodeMirror.keyMap.sublime);


Could you delete this console.log and the commented out code?

…-shortcut

vulongphan · 2021-01-21T13:09:31Z

I have made the changes accordingly, could you please have a look?

Thank you!

vulongphan · 2021-02-02T16:53:14Z

@catarak I am sorry but does this mean that this PR should be closed now? Please correct me if I missed something here

catarak · 2021-02-02T20:19:30Z

@vulongphan I just added a comment about using KeyboardEvent.metaKey, KeyboardEvent.shiftKey, and KeyboardEvent.ctrlKey—could you use these rather than handleKey?

vulongphan · 2021-02-03T08:01:51Z

@catarak It was my bad I did not see your comment earlier on. Also please ignore the first two commits (e53f956 and 0be0c85) and check 059b171.
I am really sorry for the inconvenience.

catarak · 2021-02-03T18:15:18Z

@catarak It was my bad I did not see your comment earlier on. Also please ignore the first two commits (e53f956 and 0be0c85) and check 059b171.
I am really sorry for the inconvenience.

No need to apologize!!! Contributing is about learning rather than being perfect 😄

catarak · 2021-02-03T18:17:33Z

client/modules/IDE/components/Editor.jsx

-      // 9 === Tab
-      if (e.keyCode === 9 && e.shiftKey) {
+      // 70 === f
+      if ((e.metaKey || e.ctrlKey) && e.shiftKey && e.keyCode === 70) {


i think it would also make sense to check the value of metaKey, so that Ctrl + Shift + F doesn't work on Mac.

Yes, I have added the check accordingly!

catarak · 2021-02-16T20:21:18Z

Working great for me! Would you mind updating the KeyboardShortcutModal component with the new shortcut? Thank you!

vulongphan · 2021-02-18T13:14:10Z

Hi @catarak, I have updated the file but for some reason I cannot see the change in my local development. Could you please check for me? Attached is the local development after I updated the file

catarak · 2021-02-19T22:52:07Z

I'm seeing the same thing! I think the issue is that you need to update the file Nav.jsx as well.

…-shortcut Merge upstream/develop

…phan/p5.js-web-editor into update/tidy-code-shortcut Pulled from origin update/tidy-code-shortcut

vulongphan · 2021-02-20T10:35:27Z

@catarak yes I really do agree that we have to update Nav.jsx as well. I have updated the file and now see the changes in my local development.
However, npm run test has failed which does not seem to be caused by the changes I have made. Could you please give me some suggestion?

Thank you!

catarak · 2021-02-24T18:47:37Z

Because you're changing the appearance of Nav.jsx, you need to update the snapshot as well: npm run test -- --updateSnapshot. I'm going to go ahead and just do this so I can merge this fix in.

Add Cmd+Shift+F astidy code shortcut and prevent browser default beha…

ecefaa8

…viour

catarak requested changes Jan 20, 2021

View reviewed changes

vulongphan added 2 commits January 21, 2021 17:05

Merge remote-tracking branch 'upstream/develop' into update/tidy-code…

25f3ffa

…-shortcut

make use of variable metaKey

f801d1f

Merge develop

0ef3880

vulongphan added 3 commits February 3, 2021 11:33

use KeyboardEvent to check for shortcut to tidyCode

e53f956

use KeyboardEvent to check for shortcut to tidyCode

0be0c85

use KeyboardEvent to check for shortcut to tidyCode

059b171

catarak requested changes Feb 3, 2021

View reviewed changes

use metaKey to check for Windows/MacOs

afec089

update TidyCode shortcut modal

7005999

Merge branch 'develop' into update/tidy-code-shortcut

e3ea72a

vulongphan added 3 commits February 20, 2021 13:58

Merge remote-tracking branch 'upstream/develop' into update/tidy-code…

38db3b0

…-shortcut Merge upstream/develop

update Nav bar with the short cut for tidy code

11f27f9

Merge branch 'update/tidy-code-shortcut' of https://github.com/vulong…

1cb1aa6

…phan/p5.js-web-editor into update/tidy-code-shortcut Pulled from origin update/tidy-code-shortcut

[processing#1711] Update Nav snapshot

3cef359

catarak approved these changes Feb 24, 2021

View reviewed changes

catarak merged commit 0e917dd into processing:develop Feb 24, 2021


		// delete CodeMirror.keyMap.sublime['Shift-Cmd-F'];

		console.log(CodeMirror.keyMap.sublime);

Uh oh!

Add Cmd+Shift+F as tidy code shortcut and prevent browser default behaviour #1732

Add Cmd+Shift+F as tidy code shortcut and prevent browser default behaviour #1732

Uh oh!

Conversation

vulongphan commented Jan 8, 2021

Uh oh!

vulongphan commented Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vulongphan commented Jan 21, 2021

Uh oh!

vulongphan commented Feb 2, 2021

Uh oh!

catarak commented Feb 2, 2021

Uh oh!

vulongphan commented Feb 3, 2021

Uh oh!

catarak commented Feb 3, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

catarak commented Feb 16, 2021

Uh oh!

vulongphan commented Feb 18, 2021

Uh oh!

catarak commented Feb 19, 2021

Uh oh!

vulongphan commented Feb 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

catarak commented Feb 24, 2021

Uh oh!

Uh oh!

vulongphan commented Jan 8, 2021 •

edited

Loading

vulongphan commented Feb 20, 2021 •

edited

Loading