Skip to content

feat: add rtf functionality #560

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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

feat: add rtf functionality #560

wants to merge 15 commits into from

Conversation

asanehisa
Copy link
Contributor

@asanehisa asanehisa commented Jun 24, 2025

Slightly adjusted code to better match alpha changes (alpha changes not out yet) but having these visual-editor changes with no alpha changes works fine (works as string).

Screen.Recording.2025-06-25.at.3.17.53.PM.mov

Copy link
Contributor

Warning: Component files have been updated but no migrations have been added. See https://github.com/yext/visual-editor/blob/main/packages/visual-editor/src/components/migrations/README.md for more information.

@asanehisa asanehisa marked this pull request as ready for review June 25, 2025 19:21
@mkilpatrick
Copy link
Collaborator

Does this work with translations? Aka if you change locale then you can have a different rtf?

@asanehisa
Copy link
Contributor Author

Does this work with translations? Aka if you change locale then you can have a different rtf?

Yup it does!

@asanehisa asanehisa requested a review from benlife5 June 26, 2025 20:56
if (payload.locale && payload.rtfJson) {
handleNewValue(payload.rtfJson, payload.locale, payload.rtfHtml);
} else {
// Fallback for backward compatibility
Copy link
Collaborator

Choose a reason for hiding this comment

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

Once Storm is deployed it will always send the new payload right? If so then you shouldn't need any fallback.

try {
if (!translatableRichText) return "";

if (
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's this if condition, the one below, and then the logic in isRichText which also checks if the type is an object like on line 55. Can this be simplified to just check if translatableRichText is a map, grab the locale, and then always call toStringOrElement which happens in both of these conditions?

return value ?? "";

// Ensure we only return strings
if (typeof value === "string") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, we can't migrate the strings to rtf because we don't know the formatting and that happens in Storm, right? But now the rtf fields from Storm that we're going to render are always the html version so can't we just always pretend it's a string we need to render? Or is this to support the json version from the document because Spruce hasn't switch it over yet?

Basically I'm trying to see if we can simplify a lot of the crazy logic in here. Is that possible? If not now, will it be possible at some point, and if so, what is needed to do that?

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.

3 participants