fix(RichTextInput): deserialization of <sup> and <sub>
#2523
Conversation
🦋 Changeset detectedLatest commit: f2f725f The changes in this PR will be included in the next version bump. This PR includes changesets to release 93 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
kark
force-pushed
the
kk-change-root-node-rich-text-input
branch
from
254a5a8 to
eb896ce
Compare
| el: HTMLElement | ChildNode, | ||
| textContent: TElement | TText | (TElement | TText)[] | ||
| ) => | ||
| el.parentNode?.nodeName === 'BODY' // root element, because body is eventually turned to React fragment | ||
| ? wrapWithParagraph(textContent) | ||
| ? wrapWithFragment(textContent) |
There was a problem hiding this comment.
Would this be a problem in case of new line?
In the below screenshot, there are two individual elements and above condition would be true since the parentNode is 'BODY' in this case, isn't it?
This would make the text to be in the single line without any space between them if the <p> tag is removed.
There was a problem hiding this comment.
Thanks,
I'm not sure I 100% understand the question but let me try to answer.
You're right that in this case we'll have:
Body
|
|
|----P
| |
| |
| |
| +------ #text (this is first line)
|
|
+-----P
|
|
+------#text (this is second line)
This function only replaces <body> with <></>.
Therefore, if you provide:
<LocalizedRichTextInput
onChange={() => {}}
value={{
en: '<p>this is the first line</p><p>this is the second line</p>',
}}
selectedLanguage="en"
horizontalConstraint={7}
defaultExpandMultilineText={true}
/>the wrapping paragraphs are there.
I don't get the part how do you expect the <p>s to be removed? They will always be there if this content is typed in the input by the user.
There was a problem hiding this comment.
Thanks for the detailed explanation. I understand what you mean.
However I am confused since you linked this issue which talks about removing <p> tags from unformatted text content. So does this PR removes <p> only if there is only one <p> tag at the top level?
This was my overall assumption.
If users type anything in rich text editor without formatting (i.e, not clicking bold or italic, etc) then text will not be wrapped by any elements like <p>.
If the users type anything and format their content, then respective HTML elements will be wrapped.
Is my understanding or expectations correct or is this something not possible to achieve it?
There was a problem hiding this comment.
Thanks again,
If users type anything in rich text editor without formatting (i.e, not clicking bold or italic, etc) then text will not be wrapped by any elements like
<p>.
Got it. Not sure, I need to check that.
There was a problem hiding this comment.
If users type anything in rich text editor without formatting (i.e, not clicking bold or italic, etc) then text will not be wrapped by any elements like
<p>.
If the users type anything and format their content, then respective HTML elements will be wrapped.
Is my understanding or expectations correct or is this something not possible to achieve it?
To my understanding, a richt-text input always emits HTML and always wraps the content into an element by default. For instance, typing just some text is considered a paragraph thus wrapping it into a <p> is in theory correct.
There was a problem hiding this comment.
I’m not sure about this one, but since the component is formatting text, I believe it needs to output some kind of rich information where we don’t only get the content but also its structure and styles. I mean, we need to know if a text is part of a list, if it’s a header, etc.
For what I can see in other libraries, the output uses to be HTML (which includes content, structure and styles) or JSON where all of those can be represented (example).
If the first element in the output is a list, I guess we wouldn’t be questioning if we want it to be wrapped in a ul/ol.
I’m not sure I understand the underlying problem.
kark
force-pushed
the
kk-change-root-node-rich-text-input
branch
from
eb896ce to
e0df0c6
Compare
kark
changed the title
<sup> and <sub>
kark
force-pushed
the
kk-change-root-node-rich-text-input
branch
from
1ba547b to
c2aa1bb
Compare
| editor.apply({ | ||
| type: 'insert_node', | ||
| path: [0], | ||
| node: newState[0], |
There was a problem hiding this comment.
Previously the implementation was buggy, as only the first provided node was added when ref.current.resetValue function was called
| // each non-empty text node must be wrapped with a paragraph | ||
| return children.map((child) => | ||
| Text.isText(child) && child.text ? wrapWithParagraph(child) : child | ||
| ); |
There was a problem hiding this comment.
This is not needed, it was added as a hot fix of a bug, but text nodes can in fact be added without a wrapping paragraph. This was added here but this approach was not correct as I think of it in retrospect.
| (Array.isArray(value) && | ||
| value.every((node) => SlateElement.isElement(node) || Text.isText(node))) |
There was a problem hiding this comment.
This is a valid condition now, since text nodes don't need a wrapping paragraph
| SUP: () => ({ superscript: true }), | ||
| SUB: () => ({ subscript: true }), |
There was a problem hiding this comment.
This is actually the missing piece that is a root cause of the bug
| ); | ||
| }); | ||
| }); | ||
| }); | ||
| describe('multiple nested text nodes within parent text node', () => { |
kark
force-pushed
the
kk-change-root-node-rich-text-input
branch
from
6b441ec to
f2f725f
Compare
Summary
This PR is meant to fix:
<sup>and<sub>deserialization<RichTextInput>andLocalizedRichTextInput>