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

feat(richtext-lexical): add indent and align converter utilities #8030

Closed
wants to merge 6 commits into from

Conversation

SimYunSup
Copy link
Contributor

Description

Add utilities for indent and align converter.

  1. Added getElementNodeDefaultStyle.ts in features\shared folder because it seems to be used only inside the features folder. If I'm misinterpreting this, you can change the folder location.
  2. Fixed EditorTheme.scss because --lexical-indent-base-value compiles to base(2) now
  • I have read and understand the CONTRIBUTING.md document in this repository.

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist:

  • I have added tests that prove my fix is effective or that my feature works
  • Existing test suite passes locally with my changes
  • I have made corresponding changes to the documentation

@SimYunSup SimYunSup changed the title feat(lexical): Add indent and align converter utilities feat(richtext-lexical): Add indent and align converter utilities Sep 2, 2024
@SimYunSup
Copy link
Contributor Author

Fixes #5146 , Related #5814

@GermanJablo GermanJablo changed the title feat(richtext-lexical): Add indent and align converter utilities feat(richtext-lexical): add indent and align converter utilities Sep 17, 2024
Copy link
Contributor

@GermanJablo GermanJablo left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to do this @SimYunSup and sorry for the delay in responding.

Since you made that PR, we're now using 40px multipliers for indents, just like Lexical, to follow the spacing the W3 spec uses for list-items (reference).

As Alessio said, the style attribute is highly opinionated so we want to make sure that the converter only considers these two inline styles:

  • text-align
  • padding-inline-start
  • I have added tests that prove my fix is effective or that my feature works

It seems that tests are missing. If possible, some comments explaining the changes would also be very helpful in speeding up the review!

I would say that we should have tests in both directions:
Lexical -> HTML and HTML <- Lexical.

@SimYunSup
Copy link
Contributor Author

Okay, I'll add a test for this within 2 days and update this PR.

@SimYunSup
Copy link
Contributor Author

Add tests with some fixes.

  • Delete indent for listitems(I forgot align is needed for listitem. I'll add it soon).
  • Add QuoteNode/HeadingNode wrapper because of no logic for HTML to Lexical Data.

@GermanJablo
Copy link
Contributor

Ok I've done my research and I think we need to reconsider whether this should be addressed in the Lexical repository.

The Lexical team has decided not to support highly opinionated styles such as text background-color, but they seem to be on the same page with us regarding text-align and indent. If it hasn't been fully implemented by now, it's probably due to lack of time.

Here I have documented some of the milestones in this issue.

Date PR / Issue Description
Aug 2022 PR 2785 (closed) Export text-align for Paragraph (closed)
May 2023 PR 4445 Export text-align for Heading y Quote
June 2023 PR 4625 Import text-align for Heading y Blockquote.
Mar 2023 Issue 4243 text-align for list-items (headings done)
May 2024 Issue 6082 Indent for ParagraphNode

Note that the two issues (4243 and 6082) are still open.

If the solution was a small patch it would be easier for me to consider addressing it in Payload, but I think we can agree that overwriting the original nodes is a big deal.

Sorry for the time it took me to come to this conclusion, but on the bright side I think the time you've spent on this can be partially transferred to Lexical if you decide to address the problem there.

@SimYunSup
Copy link
Contributor Author

It seems wrong to implement Lexical's issue detection first, when it should have been done first. sorry.

If we don't overwrite the node, I think we should implement to block AlignFeature/IndentFeature in quote and list-item until lexical teams are implemented about quote/list-item in lexical.

I agree it's a big deal, but I think the issue that AlignFeature/IndentFeature is not working from the output HTML will come up constantly, and I think it should be documented or some other workaround.

Once payload teams decided on a workaround, I'll delete the wrapper node and change the code in that direction.

@GermanJablo
Copy link
Contributor

Indeed it is an issue with a lot of demand so it is very high on my priority list.

The correct plan of action would be to do the PR in Lexical, making sure that all nodes properly convert indents and text-align.

If you want, you can try it yourself. Otherwise, I think I can do it on Monday.

@SimYunSup
Copy link
Contributor Author

I deleted the Wrapper Node.

I'll be working on inserting Indent logic into the importDOM logic (and maybe exportDOM logic) of QuoteNode, HeadingNode, and ListItemNode in Lexical starting tomorrow local time, but I'm not sure if I create PR and merged quickly.

@GermanJablo GermanJablo self-assigned this Sep 25, 2024
@GermanJablo
Copy link
Contributor

I've opened a PR fixing the indent issue here: facebook/lexical#6693

I'll do the same for text-align soon.

If you're interested, contributions are still welcome to make HTML serialization take into account custom indentation values ​​in editorConfig.theme.indent

@GermanJablo GermanJablo closed this Oct 3, 2024
@SimYunSup
Copy link
Contributor Author

SimYunSup commented Oct 5, 2024

I have a question:

I think this PR is the PR that implements exportDOM and importDOM, but currently @payloadcms/richtext-lexical does not implement lexicalHTML via exportDOM, so will you change the implementation of lexicalHTML?

I don't think the PR should be closed because the original issue was about HTMLConverter not applying align and indent CSS in lexicalHTML.

Additonal: I'm sorry I didn't get back to you sooner, I'm not making PRs.

@GermanJablo
Copy link
Contributor

You are correct that Payload is not using Lexical's serializer. I've spoken to @AlessioGr about this, and we'll probably change it.
If not, we'll revisit an approach like the one in this PR again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants