Table Cell Background Color #4306
Conversation
zurfyx
requested review from
fantactuka,
acywatson,
tylerjbainbridge and
thegreatercurve
as code owners
facebook-github-bot
added
the
CLA Signed
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
size-limit report 📦
|
| <hr /> | ||
| </> | ||
| )} | ||
| {mergeCellButton} |
There was a problem hiding this comment.
Do we no longer need to nil check the mergeCellButton?
There was a problem hiding this comment.
Rendering a nullish component in React will not render it. It's a shorthand for foo != null ? foo : null
| buttonLabel?: string; | ||
| title?: string; | ||
| stopCloseOnClickSelf?: boolean; | ||
| color: string; |
There was a problem hiding this comment.
Would it be appropriate to define a new type for hex colour strings, making it a bit more semantically obvious when they're used as API parameters?
There was a problem hiding this comment.
What kind of type are you thinking of? Sort of like constant for colors? I didn't change the API in this case, the ColorPicker was already present but in any case we can propose it as a good-first-issue
There was a problem hiding this comment.
I meant the JS equivalent of this Swift code:
public typealias HexColor = String
Just to give a bit more semantics of what the string should be.
| </div> | ||
| <MoveWrapper | ||
| className="color-picker-saturation" | ||
| style={{backgroundColor: `hsl(${selfColor.hsv.h}, 100%, 50%)`}} |
There was a problem hiding this comment.
you've done this logic (modify an HSV colour to have 100% sat, 50% lightness) a few times; maybe factor into its own function?
There was a problem hiding this comment.
Didn't write this file, just moved the files around. Sorry if the blame is confusing
| $isRangeSelection(selection) || | ||
| DEPRECATED_$isGridSelection(selection) | ||
| ) { | ||
| const [cell] = DEPRECATED_$getNodeTriplet(selection.anchor); |
There was a problem hiding this comment.
I assume there isn't a non-deprecated alternative here?
There was a problem hiding this comment.
The idea is to kill GridSelection eventually by moving it into the Table package itself. That's why it remains as deprecated while still actively using it
| @@ -248,6 +270,10 @@ export function convertTableCellNodeElement( | |||
| ); | |||
| tableCellNode.__colSpan = domNode_.colSpan; | |||
| tableCellNode.__rowSpan = domNode_.rowSpan; | |||
| const backgroundColor = domNode_.style.backgroundColor; | |||
| if (backgroundColor !== '') { | |||
| tableCellNode.__backgroundColor = backgroundColor; | |||
There was a problem hiding this comment.
You're going to have the same problem here that all other nodes implementing styling like this do. If the editor you paste into doesn't support this feature, then the user will have no option to remove it. Even if you paste into another Lexical editor, you may end up using HTML because of a namespace mismatch.
There was a problem hiding this comment.
Makes sense - will follow up with a solution similar to #4343
| @@ -215,21 +221,22 @@ export class TableSelection { | |||
| this.gridSelection = selection; | |||
| this.isHighlightingCells = true; | |||
| this.disableHighlightStyle(); | |||
| $updateDOMForSelection(this.grid, this.gridSelection); | |||
| $updateDOMForSelection(this.editor, this.grid, this.gridSelection); | |||
Screen.Recording.2023-04-11.at.4.21.52.PM.mov