Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SF-3212 Create 'unknown' blot as fallback blot #3052

Merged
merged 2 commits into from
Mar 6, 2025

Conversation

siltomato
Copy link
Collaborator

@siltomato siltomato commented Mar 3, 2025

This PR creates a custom ScrollBlot to override Quill's scroll blot and an UnknownBlot to render when an unknown blot type is attempted.

If an unknown blot type creation is attempted, the custom ScrollBlot catches the error and creates the UnknownBlot. After the editor renders (via setTimeout()), an error is thrown displaying the name of the failed attempted blot.

UnknownBlot uses tagName = 'sf-unknown', ('span' was causing it to be incorrectly matched from a quill registry query and throw an after.appendChild not a function error).

You can test this blot by adding TNN as a resource.

Example:
image


This change is Reviewable

@siltomato siltomato added enhancement New feature or request will require testing PR should not be merged until testers confirm testing is complete labels Mar 3, 2025
Copy link

codecov bot commented Mar 3, 2025

Codecov Report

Attention: Patch coverage is 56.25000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 82.71%. Comparing base (dc4f90a) to head (5abc7cc).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...l-editor-registration/quill-formats/quill-blots.ts 53.33% 6 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3052      +/-   ##
==========================================
- Coverage   82.72%   82.71%   -0.02%     
==========================================
  Files         563      563              
  Lines       32653    32669      +16     
  Branches     5282     5284       +2     
==========================================
+ Hits        27012    27021       +9     
- Misses       4862     4868       +6     
- Partials      779      780       +1     

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

@siltomato siltomato marked this pull request as ready for review March 3, 2025 02:12
@siltomato siltomato removed the enhancement New feature or request label Mar 3, 2025
@josephmyers josephmyers self-assigned this Mar 3, 2025
@josephmyers
Copy link
Collaborator

I haven't done a full review yet, but I think it's less than ideal to utilize the generic exception dialog for this, since that's typically reserved for errors that aren't handled, as a last resort. Per the JIRA description, we want to be notified of new blots that we need to support; however, we can be a little more elegant than the last-resort exception dialog. You could:

  • Log an error to the console for the least intrusion
  • Show an error "notice" as a middle ground

I'd think the former, since a dialog or even an error notice showing every single time would get pretty old pretty fast to users. What do you think?

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.

Yes, I think you are right. The error dialog does get old fast. The idea was to alert (still rendering the text) so that the missing blot could be remedied, but perhaps the visual presence in the text is sufficient (along with logging to the console).

Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on @josephmyers)

@siltomato siltomato force-pushed the fix/sf-3212-handle-unknown-blots branch from 1526ac2 to 0016b29 Compare March 3, 2025 19:00
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: Thanks. I've tested this with resources and projects, and it seems to handle things as expected. Awesome!

Reviewed 3 of 4 files at r1, 1 of 1 files at r2, 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 Mar 4, 2025
@RaymondLuong3 RaymondLuong3 force-pushed the fix/sf-3212-handle-unknown-blots branch from 0016b29 to 5abc7cc Compare March 6, 2025 16:57
@RaymondLuong3 RaymondLuong3 merged commit 709c2d8 into master Mar 6, 2025
18 checks passed
@RaymondLuong3 RaymondLuong3 deleted the fix/sf-3212-handle-unknown-blots branch March 6, 2025 17:15
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.

3 participants