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

refactor(portable-text-editor): automatically resolve some validation errors #6705

Merged
merged 13 commits into from
May 22, 2024

Conversation

skogsmaskin
Copy link
Member

@skogsmaskin skogsmaskin commented May 16, 2024

Description

This will introduce a new prop to validation errors resolutions: autoResolve

When a validation error occurs with such a resolution, the error will be automatically fixed for the user.

Currently the following validation errors will be automatically fixed:

  • Missing or .markDef prop on a text block. - EDIT: should support missing markDefs prop
  • Missing _key on span nods
  • Empty children array for text blocks.

These errors will be fixed through patches directly, meaning that they can be resolved without doing it through transformations in the editor. This allows for resolving the errors progressively so that the editor finally can be rendered when all the errors are resolved.

Besides this, I have also added editor normalization for the following inside the editor:

  • Remove orphaned annotation marks
  • Remove decorator marks not supported by the schema.

These will be normalized whenever an edit occurs to that node.

I have also refactored the addAnnotation static function because everything there now has to be called within the withouthNormalizing scope in order not to trigger the above mentioned normalization (in flight as we build the new annotation).

What to review

That the changes are not causing edits when a user is only opening the editor.
Nothing should be patched automatically unless the user actually edits in the editor.

Testing

Should be covered by automatic tests.

However it would be nice with some manual testing as well. You can do so by for instance using the Vision plugin to set a block to certain value, for example without markDefs or with some mark that doesn't exist.

Notes for release

  • Added automatically resolving of some validation errors in the Portable Text Editor

This was too damn messy and contained a bug where sometimes an annotation would not be added.
Fixed the bug.
…efs to editor value

We will normalize this automatically and update the value when needed. For this to work, the value must reflect the original value
…n the the schema

Note: you need to actively edit the node before it's normalized
Copy link

vercel bot commented May 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
performance-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 21, 2024 0:31am
test-next-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 21, 2024 0:31am
test-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 21, 2024 0:31am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
studio-workshop ⬜️ Ignored (Inspect) Visit Preview May 21, 2024 0:31am

@skogsmaskin skogsmaskin changed the title Edx 1291 refactor(portable-text-editor): automatically resolve some validation errors May 16, 2024
Copy link
Contributor

No changes to documentation

Copy link
Contributor

github-actions bot commented May 16, 2024

Component Testing Report Updated May 21, 2024 12:36 PM (UTC)

File Status Duration Passed Skipped Failed
comments/CommentInput.spec.tsx ✅ Passed (Inspect) 34s 15 0 0
formBuilder/ArrayInput.spec.tsx ✅ Passed (Inspect) 6s 3 0 0
formBuilder/inputs/PortableText/Annotations.spec.tsx ✅ Passed (Inspect) 27s 6 0 0
formBuilder/inputs/PortableText/copyPaste/CopyPaste.spec.tsx ✅ Passed (Inspect) 32s 11 7 0
formBuilder/inputs/PortableText/Decorators.spec.tsx ✅ Passed (Inspect) 14s 6 0 0
formBuilder/inputs/PortableText/FocusTracking.spec.tsx ✅ Passed (Inspect) 37s 15 0 0
formBuilder/inputs/PortableText/Input.spec.tsx ✅ Passed (Inspect) 1m 16s 20 0 0
formBuilder/inputs/PortableText/ObjectBlock.spec.tsx ✅ Passed (Inspect) 1m 3s 18 0 0
formBuilder/inputs/PortableText/PresenceCursors.spec.tsx ✅ Passed (Inspect) 7s 3 9 0
formBuilder/inputs/PortableText/RangeDecoration.spec.tsx ✅ Passed (Inspect) 20s 9 0 0
formBuilder/inputs/PortableText/Styles.spec.tsx ✅ Passed (Inspect) 14s 6 0 0
formBuilder/inputs/PortableText/Toolbar.spec.tsx ✅ Passed (Inspect) 29s 12 0 0

… error

Resolve errors for missing .markDefs, orphapned marks, empty children, missing child keys
@skogsmaskin skogsmaskin marked this pull request as ready for review May 16, 2024 17:59
@skogsmaskin skogsmaskin requested a review from a team May 16, 2024 17:59
@skogsmaskin skogsmaskin requested a review from a team as a code owner May 16, 2024 17:59
@skogsmaskin skogsmaskin requested review from juice49 and removed request for a team and juice49 May 16, 2024 17:59
As .markDefs is optional, return true when missing
It should be allowed and is a optional property according to the schema
@skogsmaskin
Copy link
Member Author

skogsmaskin commented May 21, 2024

I have removed the validation for missing .markDefs as this propery is optional as defined by the schema. Updated some code to reflect this.

@@ -23,7 +23,7 @@ export function isPortableTextTextBlock<T = PortableTextSpan | PortableTextObjec
value.children.every((child) => isRecord(child)) &&
('markDefs' in value // optional property
? Array.isArray(value.markDefs) && value.markDefs.every((def) => isRecord(def))
: false) &&
: true) &&
Copy link
Member Author

Choose a reason for hiding this comment

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

This was incorrectly set to false. Should be true.

Copy link
Contributor

@pedrobonamin pedrobonamin left a comment

Choose a reason for hiding this comment

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

Thanks @skogsmaskin looks good to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants