Improve undo/redo for Title component #1197

chia-yh · 2023-06-22T08:29:09Z

Summary:

Changes Made:

add title-editor.component, which uses the UndoRedo model to handle the undo / redo logic for the Title component in the same manner as comment-editor.component handles undo / redo logic in the description, but without support for image uploads and ctrl+i, ctrl+b
modify new-issue.component, title.component to use TitleEditorComponent
add title-editor.component.spec.ts for testing TitleEditorComponent

Proposed Commit Message:

Improve undo/redo for Title component

Undo/redo logic for Title component is inconsistent with the logic for
description.

Let's improve undo/redo for the Title component using the UndoRedo model
to standardise the undo/redo logic for title & description.

Eclipse-Dominator

Overall works as intended! Good work implementing the features. It is probably a good idea to abstract out the title-editor as a separate component!

Some notes:
I noticed that the edit button text is no longer centralised after the PR.

src/app/shared/title-editor/title-editor.module.ts

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

tests/app/shared/title-editor/title-editor.component.spec.ts

chia-yh · 2023-06-23T03:43:52Z

Overall works as intended! Good work implementing the features. It is probably a good idea to abstract out the title-editor as a separate component!

Some notes: I noticed that the edit button text is no longer centralised after the PR.

Thanks for catching this! I missed it, I'll try and fix it so that it's centralised like before.

codecov-commenter · 2023-06-23T13:25:17Z

Codecov Report

Patch coverage: 50.87% and project coverage change: +0.02 🎉

Comparison is base (94134a8) 54.63% compared to head (269c52b) 54.66%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1197      +/-   ##
==========================================
+ Coverage   54.63%   54.66%   +0.02%     
==========================================
  Files         101      102       +1     
  Lines        2888     2938      +50     
  Branches      535      548      +13     
==========================================
+ Hits         1578     1606      +28     
- Misses        967      975       +8     
- Partials      343      357      +14

Impacted Files	Coverage Δ
src/app/core/models/undoredo.model.ts	`73.21% <0.00%> (-4.79%)`	⬇️
.../shared/comment-editor/comment-editor.component.ts	`16.84% <0.00%> (+0.53%)`	⬆️
...hared/issue/title-editor/title-editor.component.ts	`54.00% <54.00%> (ø)`
...ase-bug-reporting/new-issue/new-issue.component.ts	`54.54% <100.00%> (ø)`
src/app/shared/issue/title/title.component.ts	`76.92% <100.00%> (ø)`

... and 1 file with indirect coverage changes

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

Eclipse-Dominator

LGTM!

Let's wait for others to review this.

damithc · 2023-06-26T15:20:17Z

@chia-yh thanks for taking up this issue. Now that we have the fix in view, we can consider if the additional code is worth the payback. Seems like it requires a lot of code changes, including a lot of new code. Is it worth the benefit? What do you all think? If the answer is not a confident 'yes', we can close this PR without merging, but still give @chia-yh credit for the work done.

chunweii

Overall a nice PR that uses the new undoredo model! I think it is okay to merge this after resolving my comments

chunweii · 2023-06-27T00:20:04Z

src/app/shared/issue/title-editor/title-editor.component.ts

+  private undo(): void {
+    const entry = this.history.undo();
+    if (entry === null) {
+      return;
+    }
+    this.titleField.setValue(entry.text);
+    this.titleInput.nativeElement.setSelectionRange(entry.selectStart, entry.selectEnd);
+  }
+
+  private redo(): void {
+    const entry = this.history.redo();
+    if (entry === null) {
+      return;
+    }
+    this.titleInput.nativeElement.value = entry.text;
+    this.titleInput.nativeElement.setSelectionRange(entry.selectStart, entry.selectEnd);
+  }
+
+  private isUndo(event: KeyboardEvent) {
+    // prevents undo from firing when ctrl shift z is pressed
+    if (navigator.platform.indexOf('Mac') === 0) {
+      return event.metaKey && event.code === 'KeyZ' && !event.shiftKey;
+    }
+    return event.ctrlKey && event.code === 'KeyZ' && !event.shiftKey;
+  }
+
+  private isRedo(event: KeyboardEvent) {
+    if (navigator.platform.indexOf('Mac') === 0) {
+      return event.metaKey && event.shiftKey && event.code === 'KeyZ';
+    }
+    return (event.ctrlKey && event.shiftKey && event.code === 'KeyZ') || (event.ctrlKey && event.code === 'KeyY');
+  }
+}


I think this code is repeated from comment-editor.component.ts. Perhaps these functions can be in undoredo.model.ts?

Good suggestion, I think isUndo and isRedo can definitely be moved into undoredo.model.ts, perhaps as a static method. However, I'm not sure if the same should be done for the undo and redo methods, as I personally feel that the handling of values from UndoRedo::undo/UndoRedo::redo should be done in the components using the UndoRedo model, in the interests of keeping UndoRedo more general. Could I perhaps ask for your thoughts on this?

Leaving the undo and redo methods as it is is okay too

tests/app/shared/title-editor/title-editor.component.spec.ts

damithc · 2023-06-27T04:16:06Z

@chunweii Is the extra code worth the benefit (as I mentioned in #1197 (comment)) ?

chia-yh · 2023-06-28T07:09:53Z

Just something I noticed while writing the additional tests in title-editor.component.spec.ts with reference to comment-editor.component.spec.ts, but I believe that some of the async tests in comment-editor.component.spec.ts might be bugged, in that the expect statements seem to never execute.

The affected tests I've found are:

describe('all buttons in the formatting toolbar add the correct markups when text box is empty', () => {
  ...
  it(`should add correct markups when the ${buttonName} button is pressed`, async () => {

describe('all buttons in the formatting toolbar add the correct markups when some text is highlighted', () => {
  ...
  it(`should add correct markups when the ${buttonName} button is pressed`, async () => {

describe('text input box', () => {
  ...
  it('should allow users to input text', async () => {

Changing the expect statements for these tests to intentionally fail doesn't raise any errors regarding failed tests when running npm run test, e.g. changing expect(textBoxDe.nativeElement.value).toEqual(expectedMarkup); to expect(textBoxDe.nativeElement.value).toEqual('');

Following the same format in title-editor.component.spec.ts, I managed to resolve this by adding await like so:

      await fixture.whenStable().then(() => {
        expect(textBox.value).toEqual('123');
      });

As this PR may be closed without being merged as the additional code may not be worth the payback, and this issue seems to be a separate concern from the issue this PR is addressing, should I create a separate issue regarding this, and fix it in a separate PR?

chunweii · 2023-06-28T16:07:31Z

As this PR may be closed without being merged as the additional code may not be worth the payback, and this issue seems to be a separate concern from the issue this PR is addressing, should I create a separate issue regarding this, and fix it in a separate PR?

Thank you for discovering this bug in our tests! You should create a separate issue and fix it in a separate PR, even if this PR is accepted

chunweii · 2023-06-28T16:13:01Z

@chunweii Is the extra code worth the benefit (as I mentioned in #1197 (comment)) ?

I think the extra code looks good to merge, but the benefit is small because there are no issues with using the default browser undo/redo for the title component. What do other developers think? @CATcher-org/2223s2

gycgabriel

I don't think the undo-redo feature will be used often for Title component in PE. It is simply easier to backspace and only applicable to Bug Reporting Phase. I also can't tell there is a change in behaviour.

Seems like it requires a lot of code changes, including a lot of new code. Is it worth the benefit? What do you all think?

I think we can merge the part of refactoring UndoRedo model. The comment-editor has too many functions, it will be good to refactor out the parts related to UndoRedo to its respective class.

gycgabriel · 2023-07-17T07:34:06Z

src/app/core/models/undoredo.model.ts

+  public static isUndo(event: KeyboardEvent) {
+    // prevents undo from firing when ctrl shift z is pressed
+    if (navigator.platform.indexOf('Mac') === 0) {
+      return event.metaKey && event.code === 'KeyZ' && !event.shiftKey;
+    }
+    return event.ctrlKey && event.code === 'KeyZ' && !event.shiftKey;
+  }
+
+  public static isRedo(event: KeyboardEvent) {
+    if (navigator.platform.indexOf('Mac') === 0) {
+      return event.metaKey && event.shiftKey && event.code === 'KeyZ';
+    }
+    return (event.ctrlKey && event.shiftKey && event.code === 'KeyZ') || (event.ctrlKey && event.code === 'KeyY');
+  }
+


This refactoring is good to have

chia-yh · 2023-07-24T01:21:32Z

I don't think the undo-redo feature will be used often for Title component in PE. It is simply easier to backspace and only applicable to Bug Reporting Phase. I also can't tell there is a change in behaviour.

Seems like it requires a lot of code changes, including a lot of new code. Is it worth the benefit? What do you all think?

I think we can merge the part of refactoring UndoRedo model. The comment-editor has too many functions, it will be good to refactor out the parts related to UndoRedo to its respective class.

I agree that the behavioral change isn't significant enough to warrant the extra code, especially since most users are unlikely to notice it.

Should I go ahead and open a new PR that refactors the UndoRedo model as mentioned? So that this PR can be closed without merging?

chia-yh · 2023-08-12T03:05:51Z

I've opened a new PR (#1205) as mentioned in a previous comment, so this PR can be closed without merging.

chia-yh added 4 commits June 22, 2023 15:01

Add title-editor.component, title-editor.module

3951d6d

Modify existing titles to use title-editor

9429a7e

Add title-editor.component.spec.ts for testing

e09cca0

Fix lint error

514f8eb

Eclipse-Dominator requested changes Jun 23, 2023

View reviewed changes

chia-yh added 2 commits June 23, 2023 11:03

Remove unused classes

b3c173a

Remove Apollo

615632e

chia-yh added 2 commits June 23, 2023 11:52

Shift title-editor component under shared/issue

3af1542

Center title-button text

ad3c2e3

Fix lint errors

88a94ea

Eclipse-Dominator approved these changes Jun 23, 2023

View reviewed changes

damithc requested review from chunweii, luminousleek and gycgabriel June 26, 2023 15:20

chunweii requested changes Jun 27, 2023

View reviewed changes

chia-yh added 3 commits June 28, 2023 14:40

Extract isUndo/isRedo methods into undoredo.model

cc4dc79

Move title-editor.component.spec.ts

8efeafb

Add additional tests for title-editor.component

b6f4b04

chunweii approved these changes Jun 28, 2023

View reviewed changes

Modify tests to account for macos

df664f9

chia-yh mentioned this pull request Jun 29, 2023

Comment editor tests pass even when they should fail #1199

Closed

Fix macos redo test

269c52b

gycgabriel reviewed Jul 17, 2023

View reviewed changes

chia-yh mentioned this pull request Aug 12, 2023

Extract isUndo, isRedo into UndoRedo model #1205

Merged

gycgabriel closed this Aug 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve undo/redo for Title component #1197

Improve undo/redo for Title component #1197

chia-yh commented Jun 22, 2023

Eclipse-Dominator left a comment

chia-yh commented Jun 23, 2023

codecov-commenter commented Jun 23, 2023 •

edited

Loading

Eclipse-Dominator left a comment •

edited

Loading

damithc commented Jun 26, 2023

chunweii left a comment

chunweii Jun 27, 2023

chia-yh Jun 27, 2023

chunweii Jun 28, 2023

damithc commented Jun 27, 2023

chia-yh commented Jun 28, 2023

chunweii commented Jun 28, 2023

chunweii commented Jun 28, 2023

gycgabriel left a comment

gycgabriel Jul 17, 2023

chia-yh commented Jul 24, 2023

chia-yh commented Aug 12, 2023

Improve undo/redo for Title component #1197

Improve undo/redo for Title component #1197

Conversation

chia-yh commented Jun 22, 2023

Summary:

Changes Made:

Proposed Commit Message:

Eclipse-Dominator left a comment

Choose a reason for hiding this comment

chia-yh commented Jun 23, 2023

codecov-commenter commented Jun 23, 2023 • edited Loading

Codecov Report

Eclipse-Dominator left a comment • edited Loading

Choose a reason for hiding this comment

damithc commented Jun 26, 2023

chunweii left a comment

Choose a reason for hiding this comment

chunweii Jun 27, 2023

Choose a reason for hiding this comment

chia-yh Jun 27, 2023

Choose a reason for hiding this comment

chunweii Jun 28, 2023

Choose a reason for hiding this comment

damithc commented Jun 27, 2023

chia-yh commented Jun 28, 2023

chunweii commented Jun 28, 2023

chunweii commented Jun 28, 2023

gycgabriel left a comment

Choose a reason for hiding this comment

gycgabriel Jul 17, 2023

Choose a reason for hiding this comment

chia-yh commented Jul 24, 2023

chia-yh commented Aug 12, 2023

codecov-commenter commented Jun 23, 2023 •

edited

Loading

Eclipse-Dominator left a comment •

edited

Loading