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

#6513 - Update Editor Component UI - Toolbar Text Indenting #6608

Closed
wants to merge 4 commits into from

Conversation

Miaplacidus
Copy link
Contributor

This PR adds buttons to the editor toolbar that allow the user to input left-aligned, center-aligned, right-aligned, and justified text.

Link to Issue

Closes: #6513

Description of Changes

  • add buttons for text aligned to the left, center, right, and for justified text

Test Plan

  • go to the create thread page
  • enter and select text. When you click on one of the text alignment buttons, your selected text will be wrapped in a styled div
  • without selecting text, click on one of the text alignment buttons. A styled div will appear in the editor.

@Miaplacidus Miaplacidus marked this pull request as ready for review February 6, 2024 21:56
Copy link
Contributor

@masvelio masvelio left a comment

Choose a reason for hiding this comment

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

I am pretty confused how this supposed to work.

  1. new icons have blue border on focus - should not have one, similar to other icons
    image

  2. a - I don't think we should insert html into the editor, it does not look good and user can delete it as they might not know why this is for
    b -when I don't have text selected and click one of the icon, cursor is at the end, instead in the middle of html tag (but considering previous point, we probably should not have html tags at all in the editor)

Kapture.2024-02-07.at.12.18.19.mp4

@Miaplacidus
Copy link
Contributor Author

@masvelio
Cursor placement is left to other cards.
In addition, there is no way to align text in markdown alone. HTML in markdown is allowed and pretty common. The only way to achieve text alignment is to add HTML.

@zakhap @CowMuon @jessmart1213

@Miaplacidus
Copy link
Contributor Author

@masvelio
The blue outline on focus appears to be default Quill styling on all of the buttons. I've gone ahead and removed it.

@jnaviask
Copy link
Collaborator

jnaviask commented Feb 7, 2024

@Miaplacidus we should NOT support HTML in Markdown, for the time being. Reason is that it poses a major security risk, as it would allow me to inject arbitrary scripts into the page if I set up the correct payload. We may want to support it later once we have validation logic in place, but we thus far have no mandate from product to support any HTML features.

@lzach83
Copy link
Contributor

lzach83 commented Feb 7, 2024

@Miaplacidus we should NOT support HTML in Markdown, for the time being. Reason is that it poses a major security risk, as it would allow me to inject arbitrary scripts into the page if I set up the correct payload. We may want to support it later once we have validation logic in place, but we thus far have no mandate from product to support any HTML features.

@jnaviask Wasn't aware of this... in #5940, PR: #6375

The pasting from HackMD issues we're mainly surrounding the tables which are in that html format. This may not be directly related, however; since I completely reconstruct the pasted markdown html in my PR. But would be helpful to get 👀 on that one from you as well.

@Miaplacidus
Copy link
Contributor Author

@jnaviask
Does that mean we just close this ticket? Markdown does not have text alignment without html.

@jnaviask
Copy link
Collaborator

jnaviask commented Feb 7, 2024

@Miaplacidus we should raise the Q with product to get a clear answer.

@jnaviask
Copy link
Collaborator

jnaviask commented Feb 7, 2024

@lzach83 I don't think your PR involves inserting HTML directly into the editor -- it's fine for some markdown patterns to generate HTML on our side, but what we don't want is user-submitted HTML.

@zakhap
Copy link
Collaborator

zakhap commented Feb 8, 2024

Affirmative here. If general text-alignment requires user-inserted HTLM, we should avoid that at this time.

ONE Caveat here is TABLE Alignment where there is a standard pattern of including : colons in the table header rows. This does not need to have specific buttons in the editor for helping the user, it can be a IYKYK situation (we should support the syntax).

Screen Shot 2024-02-07 at 8 27 52 PM

@Miaplacidus
Copy link
Contributor Author

@zakhap @jnaviask @lzach83
Understood! I'll close this PR. If we ever decide to implement HTML support, we can come back to this.

@Miaplacidus Miaplacidus closed this Feb 8, 2024
@CowMuon
Copy link
Contributor

CowMuon commented Feb 8, 2024

This can be closed, per Product, but please do see Zak's caveat on nekudatayim handling.

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.

Update Editor Component UI - Toolbar Text Indenting
6 participants