Replace font size dropdown with font size entry component #5451
Conversation
Shubhankerism
requested review from
zurfyx,
fantactuka and
acywatson
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
the
CLA Signed
| patch: Record<string, string | null>, | ||
| patch: Record< | ||
| string, | ||
| string | null | ((currentStyleValue: string | null) => string) |
There was a problem hiding this comment.
The $patchStyle method can now accept a function that fetches the next value for the property, instead of just absolute values. This also separates the concern of calculating the value for a property away from the selection package.
| @@ -5,6 +5,7 @@ | |||
| * LICENSE file in the root directory of this source tree. | |||
| * | |||
| */ | |||
| import {$isCodeHighlightNode} from '@lexical/code'; | |||
There was a problem hiding this comment.
This is sort of problematic, as it creates a dependency between these two packages. If someone were to implement code highlighting differently, they wouldn't be able to do this for their node. Typically, in these situations, we try to abstract the behavior out somehow so that's it's a heuristic on the node class, like "isUnstylable" or whatever the case is here.
There was a problem hiding this comment.
Hi, thanks for the review. I removed the package interdependency by exposing isStylable method from the node.
I also tried implementing by adding a class "isUnstylable" to the node DOM, but this approach required more modification to $patchStyleText method for checking the classList for each node, like calling getElementByKey for each node. Also, required changing several exisiting tests as the codeblock DOM would change.
The first approach seemed like a more cleaner and less complicated way to get the task done with minimal code changes to existing $patchStyleText method, so went ahead with it. Please let me know if this works. :)
|
Once, you address the package inter-dependency:
|
Thanks for the review, fixed it. |
| @@ -136,6 +136,14 @@ export class CodeHighlightNode extends TextNode { | |||
| return self.__highlightType; | |||
| } | |||
|
|
|||
| /** | |||
There was a problem hiding this comment.
I don't think you need this comment in here
| @@ -440,6 +440,14 @@ export class TextNode extends LexicalNode { | |||
| return toggleTextFormatType(format, type, alignWithFormat); | |||
| } | |||
|
|
|||
| /** | |||
| * | |||
| * @returns true if the text node can be formatted, false otherwise. | |||
There was a problem hiding this comment.
@returns true if the text node supports font styling
| isStylable(): boolean { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
What about canHaveFormat and canHaveStyle so that it follows the functions and properties we already have in the Node?
There was a problem hiding this comment.
canHaveFormat it is 🙂 Thanks, Gerard!
There was a problem hiding this comment.
Thanks for the suggestion, will make the change! :)
There was a problem hiding this comment.
Sorry, I meant, should we split the functionality into two? So canHaveFormat controls the format property whereas the canHaveStyle controls the style property
|
Does |
This PR replaces the font size drop down with an font size entry component. This will allow the user to add custom font sizes ranging from
8to72. There are also increment and decrement buttons, which allow font size update relative to the current size.For font sizes:
This PR also fixes the issue where the user can format the codeblock if the selection begins outside the codeblock. The issue is illustrated below:
Font size entry component: