-
Notifications
You must be signed in to change notification settings - Fork 83
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: add rich text editor to paragraph field #7070
base: develop
Are you sure you want to change the base?
Conversation
Hey @LeonardYam - thanks for the contribution! Some quick comments on the feature itself. Features Can we remove the H and undo/redo buttons? We only really want to keep bold, italic, strikethrough, ul and ol. Also if possible, we want to keep ctrl+Z and ctrl+Y for undo/redo, but not show the buttons. Also, any update on how to accommodate links? I've played around with it and managed to paste a link over existing text to make it linked text, but I can't remove the link after that. Menu bar In its current state it seems like the menu bar has some design jank we might need to figure out how to work around. (See the video below). But this may change after removing some of the buttons. Nonetheless I'll just write my notes out - if it's not applicable anymore after updating the menu bar, can ignore. On smaller screen sizes, the menu bar can't contain all the icons and is not scrollable. We may need to figure out how to fix this. For one, we can make the icons smaller - maybe 16x16 should be good enough. We can also add some breakpoints to decide how small before the menu bar switches into a two-row format, at which point the list and undo/redo icons can go onto the second row. This will require some playing around with screen sizes and various levels of zoom. Screen.Recording.2024-02-14.at.11.45.39.AM.movSpacing Also here's a sample i just did up quickly. Everything actually looks quite nice, except some of the formatting on lists. I wonder if we can make it so that the spacing between list items is the same regardless of the level? Right now, separate points on different indentation levels have quite a large gap between them, i.e.
should have the same spacing as
Another item is that for numbered list, I wonder if we can make it 1.a.i. notation, which is fairly standard. If not, this one's not a big issue - can leave it as is. Once these core functionality points have been ironed out, I'll start looking at the code for reviews :) |
Also, tagging @staceytan1998 for design visibility. |
Hi @justynoh, I've just looked through the comments, here are the changes made:
|
Thanks! I would be a bit worried about the lack of support for Markdown links. This is not to say we have to support markdown syntax for links per se, but in the past, users can have hyperlinked text. One way of implementing this is (code stolen from a different product's RTE implementation)
In this case you can see a |
Hi @justynoh, I've added the link functionality into the editor, where users will bring up a popover for editing links. I've also fixed some buggy behaviour regarding invalid values with I believe we are finished with implementing the desired features for a rich text editor, and the final steps would involve finalizing the design/styling of the components 😊 |
Hello @LeonardYam ! Stacey here, designer on Forms! I've done the styling for the RTE component as well as the component when hyperlinking. Here are the Figma files. Let me know if you have trouble viewing them, ping me on Telegram at @Staceee ! Invite to view file should have been sent to you!! |
Problem
There are a number of fields in FormSG, including the
Paragraph
field, that accept markdown strings but do not render the markdown in the input field.This leads to a number of issues, such as:
Related: #6221
Design considerations
Roughly speaking, these are the common options for a WYSIWYG text editor. Froala and TinyMCE do not have suitable free tiers, Quill has issues with long term support, and Slate is still in beta development. Thus, I will focus my comparisons on Lexical vs Tiptap.
Lexical has a few advantages over Tiptap, such as:
However, there are a few issues with adopting Lexical into FormSG, such as:
To illustrate the difference in abstraction, let's compare the code for checking if we are selecting a link with Lexical and Tiptap.
Evidently, adopting Lexical requires a higher investment, and since a rich text editor is not a core feature, this makes Lexical unsuitable for our use-case.
Thus, Tiptap was chosen, owing to it's excellent documentation and ease of integration with the existing code-base.
Solution
Changes by file/folder:
components/RichTextEditor
- The main, editable Tiptap instance in use when editing paragraphs, along with a menu bar component containing buttons for formatting.EditParagraph.tsx
- Swap the currentTextarea
input with Tiptap and register the form validations.ParagraphField.tsx
- Render a read-only instance of Tiptap, which keeps styling consistenteditorStyles.tsx
- ChakraUI resets the styles of any native HTML elements, thus we have to add these styles back.convert-paragraph-markdown-to-html.js
- Unfortunately, Tiptap does not have built-in support for markdown input/output. Thus, we would need to manually transform the markdown string usingmarkedjs
into HTML content for Tiptap to read and edit.Current features enabled in the rich text editor:
Breaking Changes
Concerns
Security - rendering HTML content poses security issues, especially if we were to render it with
<div dangerouslySetInnerHtml={...} />
). This security risk should hopefully be mitigated by rendering our HTML in a read-only instance of Tiptap, where Prosemirror should sanitize the HTML by stripping any HTML tags not defined in the editor schema.Error states and focus events - if we try to submit an invalid value for Tiptap, React Hook Form is unable to focus on the editor input field through refs. A naive code snippet to implement this would be like so:
This approach would work in forms with a single field like
EditParagraph.tsx
, but would likely have problematic behaviour in forms with multiple fields (when both RHF and Tiptap are firing focus events).Feature Screenshot
Tests
Manual tests of the features above