Skip to content

SF-3136 Upgrade to Quill v2#2925

Merged
siltomato merged 3 commits intomasterfrom
feature/sf-3136-quill-v2-upgrade
Jan 2, 2025
Merged

SF-3136 Upgrade to Quill v2#2925
siltomato merged 3 commits intomasterfrom
feature/sf-3136-quill-v2-upgrade

Conversation

@siltomato
Copy link
Collaborator

@siltomato siltomato commented Dec 22, 2024

https://slab.com/blog/announcing-quill-2-0/
https://github.com/slab/quill/releases/tag/v2.0.0

Quill v1.3.7 (our current version) was released in September 2019, and it appears that all ongoing work, including bug fixes, is in the v2 branch.


This change is Reviewable

@siltomato siltomato added the will require testing PR should not be merged until testers confirm testing is complete label Dec 22, 2024
@siltomato siltomato force-pushed the feature/sf-3136-quill-v2-upgrade branch from 0bf4e22 to 304001a Compare December 22, 2024 22:58
@codecov
Copy link

codecov bot commented Dec 22, 2024

Codecov Report

Attention: Patch coverage is 88.27160% with 19 lines in your changes missing coverage. Please review.

Project coverage is 80.89%. Comparing base (2191d63) to head (0f939b6).
Report is 191 commits behind head on master.

Files with missing lines Patch % Lines
...e/ClientApp/src/app/shared/text/quill-scripture.ts 85.18% 7 Missing and 1 partial ⚠️
...cripture/ClientApp/src/app/core/models/text-doc.ts 20.00% 4 Missing ⚠️
...re/ClientApp/src/app/shared/text/text.component.ts 91.89% 0 Missing and 3 partials ⚠️
...ntApp/src/app/translate/editor/editor.component.ts 89.47% 1 Missing and 1 partial ⚠️
...e/ClientApp/src/app/shared/text/text-view-model.ts 96.55% 1 Missing ⚠️
...late/editor/editor-draft/editor-draft.component.ts 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2925      +/-   ##
==========================================
- Coverage   80.91%   80.89%   -0.03%     
==========================================
  Files         533      533              
  Lines       31224    31221       -3     
  Branches     5085     5088       +3     
==========================================
- Hits        25265    25255      -10     
- Misses       5198     5203       +5     
- Partials      761      763       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@josephmyers josephmyers self-assigned this Dec 23, 2024
Copy link
Collaborator

@josephmyers josephmyers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been playing around with this for the past hour or so, and I haven't seen any bugs with the upgrade, not even the highlight on restore bug I just fixed. And the code changes are much, much cleaner than I was expecting. Good job!

Reviewed 36 of 36 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @siltomato)


src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/quill-scripture.ts line 729 at r1 (raw file):

        this.quill.scrollingContainer.scrollTop = scrollTop;
        this.quill.focus();
      }, 1);

This appears to still work as before, without all this extra code. Good job simplifying!


src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/quill-scripture.ts line 39 at r1 (raw file):

    if (typeof op.retain.length === 'number') {
      return op.retain.length;

I'm not seeing any mention of the .length format in their docs, though that may not mean much. It's all { retain: 5 }. Did they add this as a new possibility? How do I get here with a breakpoint?

Copy link
Collaborator Author

@siltomato siltomato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, hopefully it'll be a smooth upgrade.

Reviewable status: 35 of 36 files reviewed, 1 unresolved discussion (waiting on @josephmyers)


src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/quill-scripture.ts line 39 at r1 (raw file):

Previously, josephmyers wrote…

I'm not seeing any mention of the .length format in their docs, though that may not mean much. It's all { retain: 5 }. Did they add this as a new possibility? How do I get here with a breakpoint?

Good catch. The typescript def for Op threw me off, and I hallucinated a length prop from looking at the docs.

Changed to throw an error if the type of retain is not a number.

Copy link
Collaborator

@josephmyers josephmyers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @siltomato)


src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/quill-scripture.ts line 854 at r2 (raw file):

        }
      } else if (op.retain != null) {
        curIndex += op.retain as number;

Did you have a reason for changing this? It should use the method you created for retain, so that we maintain consistent behavior:
curIndex += getRetainCount(op);

@siltomato siltomato force-pushed the feature/sf-3136-quill-v2-upgrade branch from b511e1e to 1be6f3c Compare December 27, 2024 19:39
Copy link
Collaborator Author

@siltomato siltomato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also added null coalesce to 0 in some places for the returned result of getRetainCount.

Reviewable status: 32 of 36 files reviewed, 2 unresolved discussions (waiting on @josephmyers)


src/SIL.XForge.Scripture/ClientApp/src/app/shared/text/quill-scripture.ts line 854 at r2 (raw file):

Previously, josephmyers wrote…

Did you have a reason for changing this? It should use the method you created for retain, so that we maintain consistent behavior:
curIndex += getRetainCount(op);

No reason. Just forgot to change it back when I was deciding whether to scrap the function.

@siltomato siltomato force-pushed the feature/sf-3136-quill-v2-upgrade branch from 1be6f3c to 92b884d Compare December 29, 2024 20:23
Copy link
Collaborator

@josephmyers josephmyers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 4 of 4 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @siltomato)

@josephmyers josephmyers added ready to test and removed will require testing PR should not be merged until testers confirm testing is complete labels Dec 30, 2024
@siltomato siltomato force-pushed the feature/sf-3136-quill-v2-upgrade branch from 92b884d to 0f939b6 Compare January 2, 2025 20:00
@siltomato siltomato merged commit c2d46c7 into master Jan 2, 2025
@siltomato siltomato deleted the feature/sf-3136-quill-v2-upgrade branch January 2, 2025 23:44
Nateowami added a commit that referenced this pull request Jan 3, 2025
Nateowami added a commit that referenced this pull request Jan 3, 2025
siltomato added a commit that referenced this pull request Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants