Skip to content

feat(richtext-lexical): text classes - #9578

Closed
GermanJablo wants to merge 13 commits into
mainfrom
feat/TextAttributeFeature
Closed

GermanJablo wants to merge 13 commits into
mainfrom
feat/TextAttributeFeature

Conversation

@GermanJablo

@GermanJablo GermanJablo commented Nov 27, 2024 •

Copy link
Copy Markdown
Contributor

This PR allows adding arbitrary classes to TextNodes, without the need to extend the node. My idea is to do the same with the rest of the nodes.

Extending a node is quite complex and has a lot of footguns. Also, we should make extending nodes the last resort to customize the editor, since 2 plugins extending the same node would not work.

Most of the time when someone extends a node, what they want is only to add a class to the node under certain conditions, so this property would allow them to achieve their goals in a simpler way.

As an example, this PR shows how a TextColorFeature could be developed in fewer lines of code and in a safer way since it would not involve breaking the code if used with another plugin that also extends TextNode (run pnpm dev _community to try it).

The TextColorFeature is a work in progress. It still has bugs and room for improvement. Let's keep the discussion of this PR only about the new TextNode with classes. The TextColorFeature will be removed from here and incorporated in a later PR.


How does this work?

The __classes property is an object where the key-value has the form prefix-suffix if the suffix is a string, or prefix if the value is of type boolean.

For example, this is how the following object would be rendered:

{
  bg-color: 'red', // the node is rendered with class `bg-color-red`
  color: 'green', // node is rendered with class `color-green`,
  collapsed: true, // node is rendered with class `collapsed`,
  hidden: false, // this does not add any class to the node
}

Also, in order to make the serialization as lightweight as possible, I filter out values that are undefined or false, or ignore the classes property if it has no entries.


Now, the million dollar question is: What if a user is already extending and replacing TextNode? Or what if they are importing TextNode to do other things?

Well, to solve this problem we have 2 options:

Option 1: we implement this feature in the Lexical repository.
I know the Lexical team has often expressed interest in reducing the complexity of customizing nodes, and I think they might be interested in this API. If this is the case, it would be the simplest option.

Option 2: we do not re-export Lexical's TextNode, but our own TextNode with the same name.
We are currently using a wildcard export, and as far as I know you can't exclude a module, so we would have to explicitly mention all modules except the nodes, something complex to maintain.
An important point is that if there is a user who (a) is not using our re-export from Lexical as he/she should and (b) is extending the nodes he imports directly from Lexical, then there would be a breaking change. Actually, this is acceptable because the error is theirs, and we are re-exporting Lexical for a reason. But still to take into account.
If we want typescript not to complain if someone tries to use our TextNode in a Lexical function (example $isTextNode), we would have to make the node extension structurally equivalent (__classes should be optional, and the getClasses and mutateClasses methods should be functions external to the class).
By way of clarification, I have made sure to test that a node replacement API string works well. See this discussion.

@payloadcms payloadcms deleted a comment from github-actions Bot Dec 4, 2024
@GermanJablo GermanJablo changed the title Feat/TextAttributeFeature feat(richtext-lexical): text classes Dec 4, 2024
Comment on lines +47 to +66
/**
* Returns an object of classes with the form `suffix: prefix`.
* For example, the key-value `bg-color: “blue”`, will cause the node to be rendered with the class `bg-color-blue`.
* This method is only for reading the classes object. If you need to mutate it use `muteClasses`.
*/
getClasses() {
const self = this.getLatest()
return self.__classes
}

/**
* Allows to mutate the object of classes whose form is `suffix: prefix`.
* For example, the key-value `bg-color: “blue”`, will cause the node to be rendered with the class `bg-color-blue`.
* @param fn A function that receives the current classes object and allows to mutate it.
*/
mutateClasses(fn: (currentClasses: MutableClasses) => void) {
const self = this.getWritable()
fn(self.__classes)
}
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the most important part of the PR, and the API that I want to introduce

@GermanJablo
GermanJablo force-pushed the feat/TextAttributeFeature branch from e919d58 to 799aadf Compare December 4, 2024 17:01
@GermanJablo
GermanJablo marked this pull request as ready for review December 4, 2024 17:10
@GermanJablo
GermanJablo requested review from AlessioGr and removed request for DanRibbens, denolfe and jmikrut December 4, 2024 17:10
@benjaminpreiss

Copy link
Copy Markdown

Hey there,

I'd like to understand these changes a little bit better, especially regarding a use case I need myself.
Would love to be able to add classes to a paragraph in lexical, as a design system I am using implements 4 different font-sizes for paragraphs.

Do these changes help with it?

I.e. markdown of the following form would help:

# Some heading
This is a normal paragraph

<span class="small">This is a small paragraph</span>

@GermanJablo

Copy link
Copy Markdown
Contributor Author

With the team we have agreed to try another approach. Both this one and the new one we are working on would allow you to do that, yes. Stay tuned!

@benjaminpreiss

Copy link
Copy Markdown

@GermanJablo excited! Would you be so kind and link / post the new pull request / issues here, so that I can follow up?

@AlbertGazizov

Copy link
Copy Markdown

@GermanJablo we faced the same need to have predefined font sized in the richtext editor. Have you found the approach to use?

@GermanJablo

Copy link
Copy Markdown
Contributor Author

Yes! This has been resolved with the State API!

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.

4 participants