[lexical-list] Bullet item color matches text color #7024
Conversation
ivailop7
requested review from
zurfyx,
fantactuka,
acywatson,
Fetz,
Sahejkm and
potatowagon
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
the
CLA Signed
size-limit report 📦
|
|
There is the pseudoclass '::marker' that can isolate the color change to the marker only, but I didn't find a nice way to set this via javascript. |
|
This strategy is easy to implement but I don't think it's correct, IIRC updateDOM for parent nodes happens before the child nodes. You couldn't do this with CSS and something like |
This sounds like a better idea. I wanted to avoid another listener just for this, but I guess even updateDom itself is just another listener implementation. Will give it a go. |
|
You may also want to use getComputedStyle since the text could be colored by means other than setting the style attribute directly (although of course that's not how the playground works today, it may be a bit more robust in other settings). |
|
Curious why lastChild vs firstChild (given it's closer to the marker) |
|
In Google Docs it like the marker color is based on what the text color was when the marker was created, but you can change it if you apply a color to its entire contents at once (e.g. changing the left half from black to blue, then changing the right half to blue, will not change the marker to blue, but selecting all of the text and changing it to blue at once will). This would mean storing the color on the list item itself and updating it accordingly, rather than just doing DOM stuff. |
I agree it should be the first node, other editors opted for last one, thus I went with this. Probably because of their design. Will polish this sometime over the weekend. |
ivailop7
force-pushed
the
bullet_colors
branch
from
b44ee80 to
af615a0
Compare
|
@etrepum @fantactuka updated the implementaion and the after video in the description. Behaviour is better, because it fixed also matching the marker size to the font-size. That said, still unsure if this is still the most efficient way. Because I have to listen to TextNode changes for the font color/font size. I'm unsure if the cost added on every TextNode 'update' is worth adding this weight. The CSS var approach didn't work, because after updating the variable, it would change the color of all bullets to the latest variable color value, and if having to override the variable value per list item in style is essentially the same overhead. |
There was a problem hiding this comment.
This seems mostly reasonable but maybe it should be exposed as a separate register function so that people can choose to use this behavior or not? They might have different ideas for how this should work and it'll be very tricky to untangle it unless it's a separate call.
packages/lexical-list/src/index.ts
Outdated
| if ($isRangeSelection(selection)) { | ||
| listItemElement.setAttribute('style', selection.style || ''); | ||
| } |
There was a problem hiding this comment.
This should probably check to see if it's the selection that created the ListItemNode, it could be from paste or whatever. I would probably just have the created and updated work the same way, with this selection case to cover scenarios where there is no text element child
There was a problem hiding this comment.
This should probably check to see if it's the selection that created the ListItemNode, it could be from paste or whatever. I would probably just have the created and updated work the same way, with this selection case to cover scenarios where there is no text element child
Implement the other things, but I'm not sure I'm getting the "if it's the selection that created the ListItemNode" part. Are you referring to handling PASTE and/or SELECTION_CHANGE commands or something else?
There was a problem hiding this comment.
A created mutation event happens the first time a node is rendered to DOM for whatever reason, so the style of the editor's current selection might not really have anything to do with that newly rendered ListItemNode. It is mostly only relevant to when you press return or some other UI action to create a new ListItemNode. It's reasonable to assume that case should pick up the style. In that case the selection will almost certainly be collapsed inside the ListItemNode.
It's hard to imagine what all of the other scenarios that would render a new ListItemNode could be, paste is certainly one of them. Selection change probably isn't.
There was a problem hiding this comment.
If you were to always copy the style of the selection to an empty ListItemNode so long as the selection is collapsed inside of it then I think it would also work well, it doesn't need to only be applied on create (haven't looked at the code again but that could possibly simplify some other stuff?)
|
Unit tests are failing, looks like they are not expecting |
added the isCollapsed and fixed the tests, it's the same logic now for create and update for ListItemNode, where if no child, take the selection, else take the first child style. IMO, it should handle Paste correctly as well without any other additions. |
| @@ -97,6 +101,49 @@ export function registerList(editor: LexicalEditor): () => void { | |||
| }, | |||
| COMMAND_PRIORITY_LOW, | |||
| ), | |||
| editor.registerMutationListener( | |||
There was a problem hiding this comment.
Using an editor.update in a mutation listener causes an update cascade, if this were directly updating the DOM style and not the node's state it wouldn't be a problem but this will cause a second reconciliation as implemented. Is there a reason why this can't work as a transform? I think it should be fine so long as you check to make sure that the style actually changed before setting it (and thus marking the node dirty again, causing a second iteration of the transform)
There was a problem hiding this comment.
Hmm, I did remove it, but it was throwing me the error that it couldn't find an activeEditor, I'll try again, in case it was an HMR thing, but I tried a few times. OK, will look into making this a transform vs mutationListener.
There was a problem hiding this comment.
well if you are reading or updating the editor from a mutation listener then you do need a read or update call, it doesn't put you in a specific editor state because there are two to choose from. Updating is something that shouldn't happen in a mutation listener for the same reason that it shouldn't happen in an update listener (an update listener is basically a mutation listener for the whole editor state)
Most text editors do bullet lists with the marker color matching the font color in that list item. Investigated a few editors and how they handle different colors in the same list item.
They do differ in behaviour quite a bit apparently.
One of the popular editors, follows the color of the last text node. Another editor, follows the color only if all the text nodes inside the bullet have the same font color. A third one doesn't support it at all.
In the end, I decided to go with the first one, it's kind of the standard behaviour most people expect. Also because it's easiest to reason about and implement. I'm also unsure if we should do first node or last node. Personally, I think the first text node makes more sense vs the last.
Before:
Screen.Recording.2025-01-07.at.23.31.29.mov
After:
Screen.Recording.2025-01-12.at.22.45.22.mov