[lexical-table] Bug Fix: get table-cell background selection color from a class #6658

hamza221 · 2024-09-22T21:51:45Z

Description

get table-cell background selection color from a class
I couldn't find where tableCellSelected is used, my guess is that it's not so I overwrote the class
and I don't think that tableCellSelectedOverwrite is a good name so I'm open for other suggestion it's there as a placeholder for now.
Closes #6655

Test plan

…m a class Signed-off-by: hamza221 <[email protected]>

vercel · 2024-09-22T21:51:50Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
lexical	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 13, 2024 4:59am
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 13, 2024 4:59am

facebook-github-bot · 2024-09-22T21:51:50Z

Hi @hamza221!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

github-actions · 2024-09-22T21:53:32Z

size-limit report 📦

Path	Size
lexical - cjs	30.85 KB (0%)
lexical - esm	30.73 KB (0%)
@lexical/rich-text - cjs	39.58 KB (0%)
@lexical/rich-text - esm	32.67 KB (0%)
@lexical/plain-text - cjs	38.22 KB (0%)
@lexical/plain-text - esm	29.93 KB (0%)
@lexical/react - cjs	41.35 KB (0%)
@lexical/react - esm	34.03 KB (0%)

facebook-github-bot · 2024-09-22T21:55:57Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

packages/lexical-playground/src/themes/PlaygroundEditorTheme.css

packages/lexical-table/src/LexicalTableSelectionHelpers.ts

…lor from a class Signed-off-by: Hamza Mahjoubi <[email protected]>

hamza221 · 2024-09-24T12:51:04Z

I left the old logic in, because I'm not sure about another way to set a default value (new to the code base)
I'll update the failing tests once the logic gets approved

ivailop7 · 2024-09-24T22:04:44Z

The CSS class is probably a leftover from the experimental table, which we deleted a year/year and a half ago. I think it's fine to reuse that CSS class, I did the same with reintroducing the hover add column/row buttons and reusing the existing CSS classes. I'm not in favour of adding an extra override CSS class, as long as you pick the CSS class name from the config.theme object use the existing CSS Class, also +1 for the test.

etrepum · 2024-09-24T22:16:09Z

I agree that an override class is not necessary. Should be up to the user to make sure their configuration of classes and styles composes correctly.

matveeva-as · 2024-10-16T09:35:14Z

Would be great to see this fix in the next release!

ivailop7 · 2024-10-16T20:00:06Z

@hamza221 are you interested in finishing this one?

hamza221 · 2024-10-16T20:37:06Z

I'll send a commit tomorrow

…lor from a class Signed-off-by: Hamza Mahjoubi <[email protected]>

hamza221 · 2024-10-18T13:33:50Z

🤞

etrepum

I think this is getting very close! I believe there's one mistake in the test changes, and that it can be simplified further by not using element.style at all

etrepum · 2024-10-21T19:26:04Z

packages/lexical-playground/__tests__/e2e/Tables.spec.mjs

-                colspan="2"
-                style="background-color: rgb(172, 206, 247); caret-color: transparent">
+                class="PlaygroundEditorTheme__tableCell PlaygroundEditorTheme__tableCellHeader PlaygroundEditorTheme__tableCellSelected"
+                rowspan="2"


This changing from colspan to rowspan seems suspicious?

Suggested change

rowspan="2"

colspan="2"

it was an accidental ctrl c +ctrl v 😥 I'll revert it

etrepum · 2024-10-21T19:28:12Z

packages/lexical-table/src/LexicalTableSelectionHelpers.ts

@@ -1089,7 +1090,7 @@ export function $updateDOMForSelection(

    if (selectedCellNodes.has(lexicalNode)) {
      cell.highlighted = true;
-      $addHighlightToDOM(editor, cell);
+      $addHighlightToDOM(editor, cell, editor._config.theme);


I don't think this signature should change when the editor is an argument to this function already, would be easier to reference editor._config.theme from inside $addHighlightToDOM

etrepum · 2024-10-21T19:29:02Z

packages/lexical-table/src/LexicalTableSelectionHelpers.ts

+function $addHighlightToDOM(
+  editor: LexicalEditor,
+  cell: TableDOMCell,
+  editorThemeClasses: EditorThemeClasses,
+): void {


Suggested change

function $addHighlightToDOM(

editor: LexicalEditor,

cell: TableDOMCell,

editorThemeClasses: EditorThemeClasses,

): void {

function $addHighlightToDOM(editor: LexicalEditor, cell: TableDOMCell): void {

const editorThemeClasses = editor._config.theme;

etrepum · 2024-10-21T19:31:14Z

packages/lexical-table/src/LexicalTableSelectionHelpers.ts

+    element.style.setProperty('background-color', `rgb(${BROWSER_BLUE_RGB})`);
  }
  element.style.setProperty('caret-color', 'transparent');


I don't think there's any reason to use style directly if we are applying classes correctly

etrepum · 2024-11-05T19:40:35Z

Were you planning to finish this PR?

hamza221 · 2024-11-11T15:39:14Z

! I believe there's one mistake in the test changes, and that it can be simplified further by not using element.style at all

I'm okay with someone else taking over, I'm Afk until 2025

etrepum · 2024-11-11T15:44:18Z

I'll take a look at finishing this after #6759

hamza221 · 2024-11-11T15:45:28Z

Thank you @etrepum

[lexical-table] Bug Fix: get tablecell background selection color fro…

4554e14

…m a class Signed-off-by: hamza221 <[email protected]>

hamza221 requested review from zurfyx, fantactuka, acywatson, Fetz, ivailop7, Sahejkm and potatowagon as code owners September 22, 2024 21:51

vercel bot deployed to Preview – lexical September 22, 2024 21:52 View deployment

vercel bot deployed to Preview – lexical-playground September 22, 2024 21:52 View deployment

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 22, 2024

etrepum reviewed Sep 23, 2024

View reviewed changes

packages/lexical-playground/src/themes/PlaygroundEditorTheme.css Outdated Show resolved Hide resolved

etrepum reviewed Sep 23, 2024

View reviewed changes

packages/lexical-table/src/LexicalTableSelectionHelpers.ts Outdated Show resolved Hide resolved

fixup! [lexical-table] Bug Fix: get tablecell background selection co…

1282ae6

…lor from a class Signed-off-by: Hamza Mahjoubi <[email protected]>

vercel bot deployed to Preview – lexical September 24, 2024 12:50 View deployment

vercel bot deployed to Preview – lexical-playground September 24, 2024 12:50 View deployment

hamza221 requested a review from etrepum September 24, 2024 12:51

Merge branch 'main' into custom-table-selction-color

2e0e668

vercel bot deployed to Preview – lexical-playground October 16, 2024 20:01 View deployment

vercel bot deployed to Preview – lexical October 16, 2024 20:01 View deployment

fixup! [lexical-table] Bug Fix: get tablecell background selection co…

df6e7c2

…lor from a class Signed-off-by: Hamza Mahjoubi <[email protected]>

vercel bot deployed to Preview – lexical October 18, 2024 13:34 View deployment

vercel bot deployed to Preview – lexical-playground October 18, 2024 13:34 View deployment

etrepum reviewed Oct 21, 2024

View reviewed changes

etrepum self-assigned this Nov 11, 2024

Merge branch 'main' into custom-table-selction-color

d190a2c

vercel bot deployed to Preview – lexical November 13, 2024 04:59 View deployment

vercel bot deployed to Preview – lexical-playground November 13, 2024 04:59 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[lexical-table] Bug Fix: get table-cell background selection color from a class #6658

[lexical-table] Bug Fix: get table-cell background selection color from a class #6658

hamza221 commented Sep 22, 2024 •

edited

Loading

vercel bot commented Sep 22, 2024 •

edited

Loading

facebook-github-bot commented Sep 22, 2024

github-actions bot commented Sep 22, 2024 •

edited

Loading

facebook-github-bot commented Sep 22, 2024

hamza221 commented Sep 24, 2024 •

edited

Loading

ivailop7 commented Sep 24, 2024

etrepum commented Sep 24, 2024

matveeva-as commented Oct 16, 2024

ivailop7 commented Oct 16, 2024

hamza221 commented Oct 16, 2024

hamza221 commented Oct 18, 2024

etrepum left a comment

etrepum Oct 21, 2024

hamza221 Oct 22, 2024 •

edited

Loading

etrepum Oct 21, 2024

etrepum Oct 21, 2024

etrepum Oct 21, 2024

etrepum commented Nov 5, 2024

hamza221 commented Nov 11, 2024

etrepum commented Nov 11, 2024

hamza221 commented Nov 11, 2024

[lexical-table] Bug Fix: get table-cell background selection color from a class #6658

Are you sure you want to change the base?

[lexical-table] Bug Fix: get table-cell background selection color from a class #6658

Conversation

hamza221 commented Sep 22, 2024 • edited Loading

Description

Test plan

vercel bot commented Sep 22, 2024 • edited Loading

facebook-github-bot commented Sep 22, 2024

Action Required

Process

github-actions bot commented Sep 22, 2024 • edited Loading

size-limit report 📦

facebook-github-bot commented Sep 22, 2024

hamza221 commented Sep 24, 2024 • edited Loading

ivailop7 commented Sep 24, 2024

etrepum commented Sep 24, 2024

matveeva-as commented Oct 16, 2024

ivailop7 commented Oct 16, 2024

hamza221 commented Oct 16, 2024

hamza221 commented Oct 18, 2024

etrepum left a comment

Choose a reason for hiding this comment

etrepum Oct 21, 2024

Choose a reason for hiding this comment

hamza221 Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

etrepum Oct 21, 2024

Choose a reason for hiding this comment

etrepum Oct 21, 2024

Choose a reason for hiding this comment

etrepum Oct 21, 2024

Choose a reason for hiding this comment

etrepum commented Nov 5, 2024

hamza221 commented Nov 11, 2024

etrepum commented Nov 11, 2024

hamza221 commented Nov 11, 2024

hamza221 commented Sep 22, 2024 •

edited

Loading

vercel bot commented Sep 22, 2024 •

edited

Loading

github-actions bot commented Sep 22, 2024 •

edited

Loading

hamza221 commented Sep 24, 2024 •

edited

Loading

hamza221 Oct 22, 2024 •

edited

Loading