search

CATcher-org / CATcher

CATcher is a software application used for peer-testing of software projects.
https://catcher-org.github.io/CATcher/
MIT License
70 stars 68 forks source link

Improve undo/redo for Title component #1197

Closed chia-yh closed 1 year ago

chia-yh commented 1 year ago

Summary:

Fixes #1176

Changes Made:

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.
chia-yh commented 1 year ago

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. image

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

codecov-commenter commented 1 year ago

Codecov Report

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

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

:exclamation: 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 ```diff @@ 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](https://app.codecov.io/gh/CATcher-org/CATcher/pull/1197?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CATcher-org) | Coverage Δ | | |---|---|---| | [src/app/core/models/undoredo.model.ts](https://app.codecov.io/gh/CATcher-org/CATcher/pull/1197?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CATcher-org#diff-c3JjL2FwcC9jb3JlL21vZGVscy91bmRvcmVkby5tb2RlbC50cw==) | `73.21% <0.00%> (-4.79%)` | :arrow_down: | | [.../shared/comment-editor/comment-editor.component.ts](https://app.codecov.io/gh/CATcher-org/CATcher/pull/1197?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CATcher-org#diff-c3JjL2FwcC9zaGFyZWQvY29tbWVudC1lZGl0b3IvY29tbWVudC1lZGl0b3IuY29tcG9uZW50LnRz) | `16.84% <0.00%> (+0.53%)` | :arrow_up: | | [...hared/issue/title-editor/title-editor.component.ts](https://app.codecov.io/gh/CATcher-org/CATcher/pull/1197?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CATcher-org#diff-c3JjL2FwcC9zaGFyZWQvaXNzdWUvdGl0bGUtZWRpdG9yL3RpdGxlLWVkaXRvci5jb21wb25lbnQudHM=) | `54.00% <54.00%> (ø)` | | | [...ase-bug-reporting/new-issue/new-issue.component.ts](https://app.codecov.io/gh/CATcher-org/CATcher/pull/1197?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CATcher-org#diff-c3JjL2FwcC9waGFzZS1idWctcmVwb3J0aW5nL25ldy1pc3N1ZS9uZXctaXNzdWUuY29tcG9uZW50LnRz) | `54.54% <100.00%> (ø)` | | | [src/app/shared/issue/title/title.component.ts](https://app.codecov.io/gh/CATcher-org/CATcher/pull/1197?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CATcher-org#diff-c3JjL2FwcC9zaGFyZWQvaXNzdWUvdGl0bGUvdGl0bGUuY29tcG9uZW50LnRz) | `76.92% <100.00%> (ø)` | | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/CATcher-org/CATcher/pull/1197/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CATcher-org)

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

damithc commented 1 year ago

@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.

damithc commented 1 year ago

@chunweii Is the extra code worth the benefit (as I mentioned in https://github.com/CATcher-org/CATcher/pull/1197#issuecomment-1607709030) ?

chia-yh commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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

chia-yh commented 1 year ago

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 commented 1 year ago

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