Add keyboard shortcuts #965

cedricongjh · 2022-07-11T19:23:19Z

Summary:

Fixes #823

Changes Made:

Add keyboard shortcuts (ctrl+B and ctrl+I) to comment editor

CATcher.3.4.2.-.CATcher-org_undefined.-.Google.Chrome.2022-07-12.03-26-14_Trim.mp4

Proposed Commit Message:

Add keyboard shortcuts to comment editor.

Currently, our comment editor does not support keyboard
shortcuts such as ctrl + b to bold, and ctrl + i to italicize. 

Let's improve the UX of the comment editor by adding this
functionality.

codecov-commenter · 2022-07-15T05:31:45Z

Codecov Report

Base: 62.31% // Head: 62.26% // Decreases project coverage by -0.05% ⚠️

Coverage data is based on head (6d6a53e) compared to base (d85e0c6).
Patch has no changes to coverable lines.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #965      +/-   ##
==========================================
- Coverage   62.31%   62.26%   -0.06%     
==========================================
  Files          86       86              
  Lines        1937     1937              
  Branches      215      215              
==========================================
- Hits         1207     1206       -1     
  Misses        695      695              
- Partials       35       36       +1

Impacted Files	Coverage Δ
src/app/shared/issue-tables/issue-sorter.ts	`43.47% <0.00%> (-4.35%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

gycgabriel

Good variable names and readability, but doing too much in one method. Would be better to break up methods for readability and maintainability

src/app/shared/comment-editor/comment-editor.component.ts

gycgabriel · 2022-07-23T03:16:41Z

src/app/shared/comment-editor/comment-editor.component.ts

+      const highlightedText = currentText.slice(selectionStart, selectionEnd);
+      const hasInsertedCharBefore = currentText.slice(selectionStart - insertedChar.length, selectionStart) === insertedChar;
+      const hasInsertedCharAfter = currentText.slice(selectionEnd, selectionEnd + insertedChar.length) === insertedChar;
+      if (hasInsertedCharBefore && hasInsertedCharAfter) {


There is a toggling being done here, this can be indicated in the private method name or in a comment. (Toggle bold/italics)

Good point @gycgabriel ,to add on I think another suggestion is to extract out this condition into another method/boolean variable e.g. 'alreadyFormatted' (Please go ahead and use another name if you prefer, I can't think of a good one 😅)

Good suggestions @gycgabriel and @kkangs0226, i've abstracted this out into a private method

gycgabriel · 2022-07-23T03:22:57Z

src/app/shared/comment-editor/comment-editor.component.ts

+        this.commentField.setValue(
+          currentText.slice(0, selectionStart) +
+            SPACE.repeat(spacesRemovedLeft) +
+            insertedChar +
+            highlightedTextTrimmed +
+            insertedChar +
+            SPACE.repeat(spacesRemovedRight) +
+            currentText.slice(selectionEnd)
+        );


Looks readable, I like that variable names make sense

gycgabriel · 2022-07-23T03:25:55Z

Please also rename your PRs with Caps in front, and ensure that your commit message meets the requirements. (e.g. full stop at end of line)

chunweii · 2022-07-23T12:56:10Z

src/app/shared/comment-editor/comment-editor.component.ts

@@ -69,6 +68,56 @@ export class CommentEditorComponent implements OnInit {
    this.commentField.setValidators([Validators.maxLength(this.maxLength)]);
  }

+  onKeyPress(event) {
+    if (event.ctrlKey) {


Normally, mac users will want to use the command ⌘ key instead of ctrl. I think we should check for the user's OS and handle the ⌘+b or ⌘+i shortcuts.

I personally don't use a Mac so other developers can comment on this too.

This is a good point, thanks for pointing it out @chunweii . Seems like we can use metaKey to handle the ⌘ key, you might want to include this in your code.

good point, @chunweii, i've included it in the code, detecting either metaKey or ctrlKey press, as OS detection seems more complicated than I expected. I think it should be ok as metaKey in windows refers to the windows key, and even so, testing on Chrome and Firefox on my windows machine, the browsers did not detect the metaKey press. It seems that its been depreciated on Firefox at least (metaKey is no longer supported on windows):

From https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/metaKey

kkangs0226

Thanks for your work, I have left a few comments 😀

kkangs0226 · 2022-07-23T15:10:14Z

src/app/shared/comment-editor/comment-editor.component.ts

@@ -69,6 +68,56 @@ export class CommentEditorComponent implements OnInit {
    this.commentField.setValidators([Validators.maxLength(this.maxLength)]);
  }

+  onKeyPress(event) {
+    if (event.ctrlKey) {


This is a good point, thanks for pointing it out @chunweii . Seems like we can use metaKey to handle the ⌘ key, you might want to include this in your code.

kkangs0226 · 2022-07-23T15:33:14Z

src/app/shared/comment-editor/comment-editor.component.ts

+      const highlightedText = currentText.slice(selectionStart, selectionEnd);
+      const hasInsertedCharBefore = currentText.slice(selectionStart - insertedChar.length, selectionStart) === insertedChar;
+      const hasInsertedCharAfter = currentText.slice(selectionEnd, selectionEnd + insertedChar.length) === insertedChar;
+      if (hasInsertedCharBefore && hasInsertedCharAfter) {


Good point @gycgabriel ,to add on I think another suggestion is to extract out this condition into another method/boolean variable e.g. 'alreadyFormatted' (Please go ahead and use another name if you prefer, I can't think of a good one 😅)

kkangs0226 · 2022-07-23T15:53:19Z

src/app/shared/comment-editor/comment-editor.component.ts

+          return;
+      }
+      const currentText = this.commentField.value;
+      const highlightedText = currentText.slice(selectionStart, selectionEnd);


Seems like we are not considering trailing/leading spaces here, could we check for spaces here as well for checking whether it is already formatted? e.g. highlighted text' example ' with space before and after will not be considered 'formatted' in this case.

ah good point @kkangs0226, i've added a private method to detect if the trimmed highlighted text is already formatted, and another method to handle that case

cedricongjh · 2022-07-31T18:22:25Z

hi @gycgabriel, @kkangs0226, @chunweii I've refactored this PR into smaller methods, as suggested, do take a look again.
I am thinking of adding unit tests for these methods after this PR is merged, do let me know if its ok, thank you!

gycgabriel

Methods shortened, and naming const are good making code very readable. Almost LGTM:

src/app/shared/comment-editor/comment-editor.component.ts

gycgabriel

LGTM

cedricongjh · 2022-08-09T18:30:47Z

hi @chunweii, i've added in the OS detection and tested it on my mac, with the help of these docs:

https://developer.mozilla.org/en-US/docs/Web/API/Navigator/platform

kkangs0226

The issue I have pointed out seems to have been fixed, thanks for your work 👍 after @chunweii approves of the changes, we can proceed to merge.

chunweii

Other than the small nitpick, LGTM 👍

There are some minor issues, but I don't think it is a big problem. For example, pressing ctrl+b followed by ctrl+i and ctrl+b will create **_**text**_**. Also, trying to bold or italicize text across multiple lines may not work. But the github comment editor also has the same issues, and it is unlikely to be an inconvenience to students.

chunweii · 2022-08-10T08:54:49Z

src/app/shared/comment-editor/comment-editor.component.ts

@@ -5,6 +5,7 @@ import * as DOMPurify from 'dompurify';
 import { ErrorHandlingService } from '../../core/services/error-handling.service';
 import { LoggingService } from '../../core/services/logging.service';
 import { FILE_TYPE_SUPPORT_ERROR, getSizeExceedErrorMsg, SUPPORTED_FILE_TYPES, UploadService } from '../../core/services/upload.service';
+const os = require('os');


We can remove this line since we are not using node js os module.

ah my bad, removed!

kkangs0226 · 2022-08-10T09:21:46Z

There are some minor issues, but I don't think it is a big problem. For example, pressing ctrl+b followed by ctrl+i and ctrl+b will create **_**text**_**.

Good point raised, this seems like something we can work on. For example, detecting whether there are repeated characters within the highlighted text, rather than just the two ends of the string.

kkangs0226 · 2022-09-19T15:27:06Z

@cedricongjh Do you think you can work on these changes in this PR? If not, we can create another issue for this and go ahead with merging this PR first as it is not critical at the moment.

cedricongjh · 2022-09-20T18:36:43Z

@cedricongjh Do you think you can work on these changes in this PR? If not, we can create another issue for this and go ahead with merging this PR first as it is not critical at the moment.

hi @kkangs0226, I think we should create another issue for this and go ahead with merging this, thanks

…add-keyboard-shortcuts

cedricongjh force-pushed the add-keyboard-shortcuts branch from 9850049 to 8f07bae Compare July 11, 2022 20:18

dingyuchen added the s.ToReview label Jul 15, 2022

gycgabriel reviewed Jul 23, 2022

View reviewed changes

chunweii reviewed Jul 23, 2022

View reviewed changes

kkangs0226 requested changes Jul 23, 2022

View reviewed changes

cedricongjh force-pushed the add-keyboard-shortcuts branch 4 times, most recently from 3852580 to 6b41416 Compare July 31, 2022 17:38

cedricongjh changed the title ~~add keyboard shortcuts~~ Add keyboard shortcuts Jul 31, 2022

cedricongjh added 2 commits August 1, 2022 01:44

add keyboard shortcuts

dabc80d

trim highlighted text when detecting if text is already formatted

9b8d77a

cedricongjh force-pushed the add-keyboard-shortcuts branch from 6b41416 to 9b8d77a Compare July 31, 2022 17:45

Trigger Build

e2a1a0c

cedricongjh requested review from chunweii, kkangs0226 and gycgabriel July 31, 2022 18:08

gycgabriel reviewed Aug 3, 2022

View reviewed changes

cedricongjh added 2 commits August 3, 2022 22:19

abstract out common code from private methods

2fe3b0c

trigger build

ad7ec1f

gycgabriel reviewed Aug 5, 2022

View reviewed changes

cedricongjh added 2 commits August 10, 2022 01:40

add OS detection and fix wrong passing of variable

e6b14f2

Trigger build

b165f75

kkangs0226 approved these changes Aug 10, 2022

View reviewed changes

chunweii requested changes Aug 10, 2022

View reviewed changes

remove os import

a0f1731

chunweii approved these changes Aug 10, 2022

View reviewed changes

Merge branch 'master' into add-keyboard-shortcuts

d89dfcb

chunweii added s.ToMerge and removed s.ToReview labels Aug 10, 2022

cedricongjh and others added 2 commits September 21, 2022 02:44

Merge branch 'master' of https://github.com/cedricongjh/CATcher into …

f256afa

…add-keyboard-shortcuts

Merge branch 'master' into add-keyboard-shortcuts

6d6a53e

gycgabriel merged commit 5c54fa9 into CATcher-org:master Oct 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add keyboard shortcuts #965

Add keyboard shortcuts #965

cedricongjh commented Jul 11, 2022 •

edited

Loading

codecov-commenter commented Jul 15, 2022 •

edited

Loading

gycgabriel left a comment

gycgabriel Jul 23, 2022

kkangs0226 Jul 23, 2022

cedricongjh Jul 31, 2022

gycgabriel Jul 23, 2022

gycgabriel commented Jul 23, 2022

chunweii Jul 23, 2022 •

edited

Loading

kkangs0226 Jul 23, 2022

cedricongjh Jul 31, 2022

kkangs0226 left a comment

kkangs0226 Jul 23, 2022

kkangs0226 Jul 23, 2022

kkangs0226 Jul 23, 2022

cedricongjh Jul 31, 2022

cedricongjh commented Jul 31, 2022

gycgabriel left a comment

gycgabriel left a comment

cedricongjh commented Aug 9, 2022

kkangs0226 left a comment •

edited

Loading

chunweii left a comment

chunweii Aug 10, 2022 •

edited

Loading

cedricongjh Aug 10, 2022

kkangs0226 commented Aug 10, 2022

kkangs0226 commented Sep 19, 2022

cedricongjh commented Sep 20, 2022

Add keyboard shortcuts #965

Add keyboard shortcuts #965

Conversation

cedricongjh commented Jul 11, 2022 • edited Loading

Summary:

Changes Made:

Proposed Commit Message:

codecov-commenter commented Jul 15, 2022 • edited Loading

Codecov Report

gycgabriel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gycgabriel commented Jul 23, 2022

chunweii Jul 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkangs0226 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cedricongjh commented Jul 31, 2022

gycgabriel left a comment

Choose a reason for hiding this comment

gycgabriel left a comment

Choose a reason for hiding this comment

cedricongjh commented Aug 9, 2022

kkangs0226 left a comment • edited Loading

Choose a reason for hiding this comment

chunweii left a comment

Choose a reason for hiding this comment

chunweii Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkangs0226 commented Aug 10, 2022

kkangs0226 commented Sep 19, 2022

cedricongjh commented Sep 20, 2022

cedricongjh commented Jul 11, 2022 •

edited

Loading

codecov-commenter commented Jul 15, 2022 •

edited

Loading

chunweii Jul 23, 2022 •

edited

Loading

kkangs0226 left a comment •

edited

Loading

chunweii Aug 10, 2022 •

edited

Loading