-
Notifications
You must be signed in to change notification settings - Fork 426
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
fix(portable-text-editor): add autoresolving validations and fix normalization issues #6770
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
No changes to documentation |
Component Testing Report Updated May 27, 2024 12:40 PM (UTC)
|
@@ -310,7 +310,7 @@ describe('initialization', () => { | |||
_key: 'abc', | |||
_type: 'myTestBlockType', | |||
markDefs: [], | |||
children: [{_key: 'def', _type: 'span', text: 'Test', marks: ['invalid']}], | |||
children: [{_key: 'def', _type: 'span', marks: []}], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous test case is now auto-resolved, so we need another (hard) validation error for this test.
}, | ||
{at: focusPath, mode: 'lowest', voids: false}, | ||
) | ||
editor.withoutNormalizing(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole transformation must be run without normalizing, as we build up the data (we don't want to be normalizing orphaned data in the intermediate state).
@@ -142,7 +142,7 @@ export function createWithInsertData( | |||
// Validate the result | |||
const validation = validateValue(parsed, schemaTypes, keyGenerator) | |||
// Bail out if it's not valid | |||
if (!validation.valid) { | |||
if (!validation.valid && !validation.resolution?.autoResolve) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validation errors for pasted fragments that can be auto-resolved should pass.
4c507fa
to
99fa9af
Compare
99fa9af
to
d7c7040
Compare
@@ -276,7 +283,7 @@ export function validateValue( | |||
i18n: { | |||
description: 'inputs.portable-text.invalid-value.orphaned-marks.description', | |||
action: 'inputs.portable-text.invalid-value.orphaned-marks.action', | |||
values: {key: blk._key, orphanedMarks}, | |||
values: {key: blk._key, orphanedMarks: orphanedMarks.map((m) => m.toString())}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Values for i18n must be strings, so safeguard this map as strings.
['split_node', 'remove_node', 'remove_text', 'merge_node'].includes(op.type), | ||
editor.isTextBlock(node) && | ||
!editor.operations.some( | ||
(op) => op.type === 'merge_node' && 'markDefs' in op.properties && op.path.length === 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of the new normalization rules, this must be rewritten (as discovered by tests).
Now we do the markDefs normalization here for every operation that is NOT merging two text blocks, as this will be an intermediate state, where potentially new child nodes are added later in the change (next operation). We should do this normalization in the later operations instead.
) | ||
) { | ||
normalizeMarkDefs(editor, types) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very pretty inefficient as it will do the whole editor, and not the actual nodes that are transformed.
@@ -517,7 +516,6 @@ export function createWithEditableAPI( | |||
}) | |||
} | |||
}) | |||
normalizeMarkDefs(editor, types) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now done in the normalization inside withPortableTextMarksModel
…alization issue This will add up front validation for orphaned marks and markDefs that can be autoresolved through patches. It will also fix some issues when it comes to adding and removing annotations, which transformations now must be built up using the withoutNormalization scope.
d7c7040
to
839e93d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great 👍
Description
This will add two auto-resolving validations:
By introducing these new validations, I found two issues. The first was there before, and the other one occured introducing the new normalization rules of markDefs.
What to review
Testing
Should be covered by unit tests.
Notes for release
Improve the validation of content in the Portable Text Editor.