facebook / lexical

Lexical is an extensible text editor framework that provides excellent reliability, accessibility and performance.
https://lexical.dev
MIT License
19.95k stars 1.7k forks source link

[lexical-playground] Fix: tabs do not show strikethrough #6811

Open vantage-ola opened 2 weeks ago

vantage-ola commented 2 weeks ago

Description

Made some changes to .PlaygroundEditorTheme__textStrikethrough to show strikethroughs on tabs

Closes #6808

Before

Screenshot 2024-11-08 161836

After selecting the whole text, it shows the strikethrough is present Screenshot 2024-11-08 162158

After

The slight change in css makes the strikethrough visible

Screenshot 2024-11-08 162320

vercel[bot] commented 2 weeks ago

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 22, 2024 5:56pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 22, 2024 5:56pm
github-actions[bot] commented 2 weeks ago

size-limit report 📦

Path Size
lexical - cjs 30.94 KB (0%)
lexical - esm 30.8 KB (0%)
@lexical/rich-text - cjs 39.6 KB (0%)
@lexical/rich-text - esm 32.66 KB (0%)
@lexical/plain-text - cjs 38.2 KB (0%)
@lexical/plain-text - esm 29.92 KB (0%)
@lexical/react - cjs 41.37 KB (0%)
@lexical/react - esm 34 KB (0%)
vantage-ola commented 1 week ago

i took another approach to fixing this.., i just jumped into it without thinking and was in an hurry to fix it... I had even forgotten both underline and strikethrough existed...

the reason I used white-space: pre; is because normally HTML collapses multiple spaces into one, butwhite-space: pre treats them like<pre> tags

vantage-ola commented 1 week ago

i took another approach to fixing this.., i just jumped into it without thinking and was in an hurry to fix it... I had even forgotten both underline and strikethrough existed...

the reason I used white-space: pre; is because normally HTML collapses multiple spaces into one, butwhite-space: pre treats them like\<pre\> tags

no need for them actually. 😅 silly me.. i was using display: inline-block; while trying out things...

vantage-ola commented 1 week ago

this should be good to go right?

vantage-ola commented 1 week ago

sorry but what does text wrap mean? do you mean the way the automatic adjustment of the text?

what is the expected behaviour?, i really cant see any issue, im sorry 😭 can you point it out for me?

etrepum commented 1 week ago

Text wrap means that the text is longer than one line, so it wraps to the next line. You can see it in the second paragraph which has strikethrough and underline, you can see not all of the text has strikethrough because the pseudo-element is not the same shape as the text it's supposed to draw a line through. The tab creates a new element so that's where the strikethrough resets and works until the next wrap.

etrepum commented 1 week ago

I've highlighted the areas that are rendered incorrectly

Screenshot 2024-11-12 at 08 45 39-annotated
vantage-ola commented 1 week ago

ohhhhhh, ok. thank you. i see it now...

bdgrichards commented 1 week ago

Thanks so much for taking a look at this! Much appreciated 🙏

Looking at the files changed will this only fix this for the Lexical playground, or for regular lexical editors as well?

bdgrichards commented 1 week ago

Also when a single word wraps multiple lines this seems to break down, since the added line renders in the middle of the two/three/n lines:

https://github.com/user-attachments/assets/cd7cb7be-4045-472d-ae7e-24d2e904c3c1

vantage-ola commented 1 week ago

@etrepum will give a better answer. but I think the issue fixes only the lexical playground. I dont know of the regular lexical editors :)

etrepum commented 1 week ago

Since the fix will be presumably be primarily CSS based, it will be up to the user to configure their theme with the correct CSS to make it work. I think perhaps in this case the best solution might be to add a class to the TabNode so that it can be specifically targeted for a workaround rather than applying a workaround to all text nodes

vantage-ola commented 6 days ago

@etrepum I havent tried all the ways i could fix this issue with css. but i am thinking of injecting a javascript function to replace regular spaces with non-breaking spaces ( ) , but several issues may arise from doing that..... what approach do you think i can take... since the pseudo-element approach doesnt fully work...

etrepum commented 6 days ago

Don't bother trying to replace the spaces, that will cause problems. I suggested a possible approach in my last comment, this workaround only needs to apply to tab nodes. If you add a class so they can be targeted you should be able to work around it for only those nodes and leave the other CSS alone so this fix doesn't break anything else.

vantage-ola commented 5 days ago

I tried soemthing... but it breaks something in the tests... but i think the strikethrough/underlining or both works, atleast from what i tested in the playground

Argument of type 'TextNode' is not assignable to parameter of type 'CodeHighlightNode | TabNode | LineBreakNode'.
      Property 'getClassName' is missing in type 'TextNode' but required in type 'TabNode'.

    265       let node = getFirstCodeNodeOfLine(firstSelectionNode);
                                                ~~~~~~~~~~~~~~~~~~

      packages/lexical/src/nodes/LexicalTabNode.ts:52:3
        52   getClassName(): string {
             ~~~~~~~~~~~~
        'getClassName' is declared here.
vantage-ola commented 2 days ago

so, only createDOM is needed in LexicalTabNode, but I noticed a number of tests are failing due to a new class name being introduced to lexical. And while testing, I dont think the pseudo element approach will ever work(but i may be wrong though)

for example:

 ● LexicalSelection tests › Paste text, move selection and delete word forward (#138)

    expect(received).toBe(expected) // Object.is equality

    Expected: "<div contenteditable=\"true\" style=\"user-select: text; white-space: pre-wrap; word-break: break-word;\" data-lexical-editor=\"true\"><p class=\"editor-paragraph\" dir=\"ltr\"><span data-lexical-text=\"true\">ABD</span><span data-lexical-text=\"true\">       </span><span data-lexical-text=\"true\">EFG</span></p></div>"
    Received: "<div contenteditable=\"true\" style=\"user-select: text; white-space: pre-wrap; word-break: break-word;\" data-lexical-editor=\"true\"><p class=\"editor-paragraph\" dir=\"ltr\"><span data-lexical-text=\"true\">ABD</span><span class=\"PlaygroundEditorTheme__tabNode\" data-lexical-text=\"true\">       </span><span data-lexical-text=\"true\">EFG</span></p></div>"
etrepum commented 1 day ago

Yes, of course the tests that use tab node will have to change when a class is added.