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.

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

Add highlight TextFormatType #3583

Merged
merged 1 commit into from
Jan 31, 2023

Conversation

moy2010
Copy link
Contributor

@moy2010 moy2010 commented Dec 17, 2022

Add a new highlight variant to TextFormatType, to cover the use case of highlighted text.

@vercel
Copy link

vercel bot commented Dec 17, 2022

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

Name Status Preview Updated
lexical ✅ Ready (Inspect) Visit Preview Dec 17, 2022 at 10:27PM (UTC)
lexical-playground ✅ Ready (Inspect) Visit Preview Dec 17, 2022 at 10:27PM (UTC)

@facebook-github-bot 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 Dec 17, 2022
@trueadm
Copy link
Collaborator

trueadm commented Dec 17, 2022

I wonder if this might come over confusing because we already have a MarkNode in @lexical/mark that uses <mark>?

@moy2010
Copy link
Contributor Author

moy2010 commented Dec 17, 2022

I wonder if this might come over confusing because we already have a MarkNode in @lexical/mark that uses <mark>?

Ah! Didn't know this use case was already covered 🙈 . I'll close the PR then.

@moy2010
Copy link
Contributor Author

moy2010 commented Dec 17, 2022

Duplicated functionality

@moy2010
Copy link
Contributor Author

moy2010 commented Dec 21, 2022

Re-opening the PR after playing with the MarkNode for a few days, and finding its usage not the best in some use cases.

@fantactuka
Copy link
Contributor

Re-opening the PR after playing with the MarkNode for a few days, and finding its usage not the best in some use cases.

Could you describe cases where existing marks are not fitting?

@moy2010
Copy link
Contributor Author

moy2010 commented Dec 21, 2022

Could you describe cases where existing marks are not fitting?

Yes, of course. The simplest one is where the user wants to re-adjust the range selection. Let's take a simple text with two words:

first second

And the user wants to highlight only first, but for accuracy reasons while selecting the text with the mouse, the user selects too much and highlights: first s. Then the user wants to unhighlight the unnecessary part ( s), selects it and hits the button to do so, but because this is being wrapped by an element node, everything will be "unhighlighted".

I know it's a silly example, but it reflects that the UX of selecting/highlighting text with an element node rather than a text node format is cumbersome.

Copy link
Collaborator

@trueadm trueadm left a comment

Choose a reason for hiding this comment

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

After thinking about this more. It makes sense for your use case. MarkNode elements do have slightly different heuristics that are more about structure than styling/formatting. Thanks for the PR.

@trueadm trueadm merged commit 77d7730 into facebook:main Jan 31, 2023
@trueadm trueadm mentioned this pull request Feb 3, 2023
@moy2010 moy2010 deleted the add-highlight-text-format-type branch October 24, 2023 19:34
bfritscher added a commit to bfritscher/lexical that referenced this pull request Aug 13, 2024
Pull request facebook#3583 by @moy2010 added a new textformat (which is helpful), but if we use HTML to serialize it gets lost because importDOM is not parsing the mark.

- Added the missing mark entry
- Added a highlight formatting menu entry to playground for manual testing (including icon, and CSS theme)
- added the missing entry in devtools to show format: highlight
- added a test to check for mark on html past
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants