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

exportDOM methods for HeadingNode, QuoteNode #4445

Merged
merged 6 commits into from May 9, 2023

Conversation

ivailop7
Copy link
Collaborator

@ivailop7 ivailop7 commented May 2, 2023

Adding exportDOM methods for the HeadingNode, QuoteNode (rich-text package). All of the changes are outside of the core, so adding the methods should be safe performance-wise.

Before:

exportDOM-pre.mp4

After:

exportDOMs-post.mp4

@vercel
Copy link

vercel bot commented May 2, 2023

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 May 5, 2023 6:00pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 5, 2023 6:00pm

@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 May 2, 2023

const indent = this.getIndent();
if (indent > 0) {
element.style.textIndent = `${indent * 20}px`;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think any of these apply to list item, but indent especially will not do what we want here, as indent has a different implementation in lists than in elements generally. Also I'm not sure we should be using textIndent anywhere, as it only indents the first line of the text.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree that it does not make sense for lists, as for textIndent for other nodes seems like this one is the explanation:

// padding-inline-start is not widely supported in email HTML, but
// Lexical Reconciler uses padding-inline-start. Using text-indent instead.
element.style.textIndent = `${indent * 20}px`;
}

Copy link
Collaborator Author

@ivailop7 ivailop7 May 5, 2023

Choose a reason for hiding this comment

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

Thank you @acywatson and @fantactuka will remove the textIndent part, I actually didn't realize it was even a thing outside of lists. What would be the correct way to solve this for lists, where should I be looking to solve the alignment on an individual list item level.

Copy link
Contributor

Choose a reason for hiding this comment

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

@fantactuka

seems like this one is the explanation:

Yea but if you look at the docs, these do completely different things:

https://developer.mozilla.org/en-US/docs/Web/CSS/text-indent

https://developer.mozilla.org/en-US/docs/Web/CSS/padding-inline-start

I don't understand how that text-indent property is working the same way as padding-inline-start.

@ivailop7

where should I be looking to solve the alignment on an individual list item level.

Alignment in what sense? List indentation is semantic rather than stylistic in Lexical.

Copy link
Collaborator Author

@ivailop7 ivailop7 May 9, 2023

Choose a reason for hiding this comment

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

as in having a single item from a list have justification "right", while the others remain with "left" justification. The bullet icon remains still correctly indented, it's just the text alignment on the row.

packages/lexical-rich-text/src/index.ts Show resolved Hide resolved
@ivailop7 ivailop7 changed the title exportDOM methods for HeadingNode, QuoteNode, ListItemNode exportDOM methods for HeadingNode, QuoteNode May 5, 2023
@ivailop7
Copy link
Collaborator Author

ivailop7 commented May 9, 2023

@acywatson Could I get a second look. I've removed the list item part.

@ivailop7 ivailop7 merged commit 88cdc6b into facebook:main May 9, 2023
39 of 45 checks passed
@zurfyx zurfyx mentioned this pull request May 23, 2023
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.

None yet

4 participants