Skip to content
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.

Sign up for GitHub

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

[lexical-list] Bullet item color matches text color #7024

Merged
merged 17 commits into from
Feb 24, 2025
Merged

[lexical-list] Bullet item color matches text color #7024

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
7080a79
Bullet color to follow text color
ivailop7 Jan 7, 2025
af615a0
Using Listeners
ivailop7 Jan 12, 2025
6b7f32c
Implement suggestions
ivailop7 Jan 14, 2025
5869d83
Check selection is collapsed
ivailop7 Jan 19, 2025
5622672
Fix test
ivailop7 Jan 19, 2025
f9f3cc2
nodetransform
ivailop7 Jan 28, 2025
e8c3774
Change ListItemNode style based on transforms and selection
etrepum Jan 28, 2025
6c1f557
Merge remote-tracking branch 'origin/main' into bullet_colors
etrepum Feb 22, 2025
60657d4
Merge remote-tracking branch 'origin/main' into bullet_colors
etrepum Feb 22, 2025
616de7e
Use textFormat and textStyle for elements
etrepum Feb 22, 2025
0ad4984
use $is functions for narrowing (fortunately $isRangeSelection accept…
etrepum Feb 22, 2025
9d2d03c
ci-check
etrepum Feb 22, 2025
cce2d48
Collab workaround
etrepum Feb 23, 2025
3040dea
more cleanup? still expecting a failure in firefox collab - want to s…
etrepum Feb 23, 2025
c6174c2
tag table merge test as flaky
etrepum Feb 24, 2025
bfffccb
Add firefox workaround
etrepum Feb 24, 2025
b5613d9
Merge branch 'main' into bullet_colors
etrepum Feb 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion packages/lexical-list/src/LexicalListItemNode.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,10 @@ export class ListItemNode extends ElementNode {
}
element.value = this.__value;
$setListItemThemeClassNames(element, config.theme, this);
const nextStyle = this.__style || this.__textStyle;
if (nextStyle) {
element.style.cssText = nextStyle;
}
return element;
}

Expand All @@ -95,7 +99,14 @@ export class ListItemNode extends ElementNode {
// @ts-expect-error - this is always HTMLListItemElement
dom.value = this.__value;
$setListItemThemeClassNames(dom, config.theme, this);

const prevStyle = prevNode.__style || prevNode.__textStyle;
const nextStyle = this.__style || this.__textStyle;
if (prevStyle !== nextStyle) {
dom.style.cssText = nextStyle;
if (nextStyle === '') {
dom.removeAttribute('style');
}
}
return false;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see, could you explain to me the general rule here. Why are we avoiding returning 'true' in updateDOM with something like (this.style != prevNode.style), but we are always ok on doing the comparison and changing this in here and returning false. Does updateDOM returning true, trigger a waterfall of updates that is unnecessary?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The check here is for performance and to avoid having style=“” show up in the DOM, it could be unconditionally set.

returning true should be avoided whenever possible, it creates a lot more work on the browser and can lose state if there’s anything ephemeral in the DOM. Same reason why react does DOM diffing instead of rendering everything from scratch, just more manual here.

}

Expand Down
32 changes: 24 additions & 8 deletions packages/lexical-list/src/formatList.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,14 @@
import {$getNearestNodeOfType} from '@lexical/utils';
import {
$createParagraphNode,
$getChildCaret,
$getSelection,
$isElementNode,
$isLeafNode,
$isRangeSelection,
$isRootOrShadowRoot,
$normalizeCaret,
$setPointFromCaret,
ElementNode,
LexicalNode,
NodeKey,
Expand Down Expand Up @@ -259,7 +262,9 @@ export function $removeList(): void {
const listItems = $getAllListItems(listNode);

for (const listItemNode of listItems) {
const paragraph = $createParagraphNode();
const paragraph = $createParagraphNode()
.setTextStyle(selection.style)
.setTextFormat(selection.format);

append(paragraph, listItemNode.getChildren());

Expand All @@ -273,10 +278,16 @@ export function $removeList(): void {
// When the corresponding listItemNode is deleted and replaced by the newly generated paragraph
// we should manually set the selection's focus and anchor to the newly generated paragraph.
if (listItemNode.__key === selection.anchor.key) {
selection.anchor.set(paragraph.getKey(), 0, 'element');
$setPointFromCaret(
selection.anchor,
$normalizeCaret($getChildCaret(paragraph, 'next')),
);
}
if (listItemNode.__key === selection.focus.key) {
selection.focus.set(paragraph.getKey(), 0, 'element');
$setPointFromCaret(
selection.focus,
$normalizeCaret($getChildCaret(paragraph, 'next')),
);
}

listItemNode.remove();
Expand Down Expand Up @@ -383,8 +394,12 @@ export function $handleIndent(listItemNode: ListItemNode): void {
// otherwise, we need to create a new nested ListNode

if ($isListNode(parent)) {
const newListItem = $createListItemNode();
const newList = $createListNode(parent.getListType());
const newListItem = $createListItemNode()
.setTextFormat(parent.getTextFormat())
.setTextStyle(parent.getTextStyle());
const newList = $createListNode(parent.getListType())
.setTextFormat(parent.getTextFormat())
.setTextStyle(parent.getTextStyle());
newListItem.append(newList);
newList.append(listItemNode);

Expand Down Expand Up @@ -506,9 +521,10 @@ export function $handleListInsertParagraph(): boolean {
} else {
return false;
}
replacementNode.setTextStyle(selection.style);
replacementNode.setTextFormat(selection.format);
replacementNode.select();
replacementNode
.setTextStyle(selection.style)
.setTextFormat(selection.format)
.select();

const nextSiblings = anchor.getNextSiblings();

Expand Down
57 changes: 48 additions & 9 deletions packages/lexical-list/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,13 @@ import type {LexicalCommand, LexicalEditor} from 'lexical';

import {mergeRegister} from '@lexical/utils';
import {
$getSelection,
$isRangeSelection,
$isTextNode,
COMMAND_PRIORITY_LOW,
createCommand,
INSERT_PARAGRAPH_COMMAND,
TextNode,
} from 'lexical';

import {
Expand Down Expand Up @@ -91,17 +95,52 @@ export function registerList(editor: LexicalEditor): () => void {
),
editor.registerCommand(
INSERT_PARAGRAPH_COMMAND,
() => {
const hasHandledInsertParagraph = $handleListInsertParagraph();

if (hasHandledInsertParagraph) {
return true;
}

return false;
},
() => $handleListInsertParagraph(),
COMMAND_PRIORITY_LOW,
),
editor.registerNodeTransform(ListItemNode, (node) => {
const firstChild = node.getFirstChild();
if (firstChild) {
if ($isTextNode(firstChild)) {
const style = firstChild.getStyle();
const format = firstChild.getFormat();
if (node.getTextStyle() !== style) {
node.setTextStyle(style);
}
if (node.getTextFormat() !== format) {
node.setTextFormat(format);
}
}
} else {
// If it's empty, check the selection
const selection = $getSelection();
if (
$isRangeSelection(selection) &&
(selection.style !== node.getTextStyle() ||
selection.format !== node.getTextFormat()) &&
selection.isCollapsed() &&
node.is(selection.anchor.getNode())
) {
node.setTextStyle(selection.style).setTextFormat(selection.format);
}
}
}),
editor.registerNodeTransform(TextNode, (node) => {
const listItemParentNode = node.getParent();
if (
$isListItemNode(listItemParentNode) &&
node.is(listItemParentNode.getFirstChild())
) {
const style = node.getStyle();
const format = node.getFormat();
if (
style !== listItemParentNode.getTextStyle() ||
format !== listItemParentNode.getTextFormat()
) {
listItemParentNode.setTextStyle(style).setTextFormat(format);
}
}
}),
);
return removeListener;
}
Expand Down
7 changes: 2 additions & 5 deletions packages/lexical-list/src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,12 +179,9 @@ export function $removeHighestEmptyListParent(
emptyListPtr.getNextSibling() == null &&
emptyListPtr.getPreviousSibling() == null
) {
const parent = emptyListPtr.getParent<ListItemNode | ListNode>();
const parent = emptyListPtr.getParent();

if (
parent == null ||
!($isListItemNode(emptyListPtr) || $isListNode(emptyListPtr))
) {
if (parent == null || !($isListItemNode(parent) || $isListNode(parent))) {
break;
}

Expand Down
5 changes: 5 additions & 0 deletions packages/lexical-playground/__tests__/e2e/Collaboration.spec.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -512,6 +512,11 @@ test.describe('Collaboration', () => {
.frameLocator('iframe[name="right"]')
.locator('[data-lexical-editor="true"]')
.focus();
// TODO this is a workaround for Firefox so that the
// selection picks up the text format
if (browserName === 'firefox') {
await page.keyboard.press('ArrowLeft', {delay: 50});
}
Comment on lines +515 to +519
Copy link
Collaborator

Choose a reason for hiding this comment

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

This requires further investigation but I don't think it should block this feature

await page.keyboard.press('ArrowDown', {delay: 50});
await page.keyboard.type(' text');

Expand Down
6 changes: 4 additions & 2 deletions packages/lexical-playground/__tests__/e2e/List.spec.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ test.describe.parallel('Nested List', () => {
<li
class="PlaygroundEditorTheme__listItem PlaygroundEditorTheme__ltr"
dir="ltr"
style="color: ${expectedColor};"
value="1">
<strong
class="PlaygroundEditorTheme__textBold"
Expand All @@ -202,11 +203,13 @@ test.describe.parallel('Nested List', () => {
</li>
<li
class="PlaygroundEditorTheme__listItem PlaygroundEditorTheme__nestedListItem"
style="color: ${expectedColor};"
value="2">
<ul class="PlaygroundEditorTheme__ul">
<li
class="PlaygroundEditorTheme__listItem PlaygroundEditorTheme__ltr"
dir="ltr"
style="color: ${expectedColor};"
value="1">
<strong
class="PlaygroundEditorTheme__textBold"
Expand All @@ -220,6 +223,7 @@ test.describe.parallel('Nested List', () => {
<li
class="PlaygroundEditorTheme__listItem PlaygroundEditorTheme__ltr"
dir="ltr"
style="color: ${expectedColor};"
value="2">
<strong
class="PlaygroundEditorTheme__textBold"
Expand Down Expand Up @@ -1898,7 +1902,6 @@ test.describe.parallel('Nested List', () => {
</p>
`,
);
await page.pause();
await assertSelection(page, {
anchorOffset: 0,
anchorPath: [1],
Expand Down Expand Up @@ -1956,7 +1959,6 @@ test.describe.parallel('Nested List', () => {
</ul>
`,
);
await page.pause();
await assertSelection(page, {
anchorOffset: 0,
anchorPath: [2],
Expand Down
Loading