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: add rich text editor to paragraph field #7070

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from

Conversation

LeonardYam
Copy link
Contributor

@LeonardYam LeonardYam commented Feb 6, 2024

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:

  1. Text formatting functionalities are restricted to those who know markdown syntax
  2. Inconsistent rendering between the text input and form output, which leads to unexpected formatting like being stuck in the list indentation level as shown below
image

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:

  1. Support for markdown input/output, which is ideal since our fields use markdown string
  2. Better long term support - it is created by Meta and has React 18+ support
  3. Better bundle size
  4. According to the creator, better performance and accessibility

However, there are a few issues with adopting Lexical into FormSG, such as:

  1. Lack of good, comprehensive documentation
  2. Steep learning curve - Lexical is more complex and analogous to Prosemirror (which Tiptap is built on), whereas Tiptap provides developer friendly abstractions.

To illustrate the difference in abstraction, let's compare the code for checking if we are selecting a link with Lexical and Tiptap.

// Lexical
const [editor] = useLexicalComposerContext();
const selection = $getSelection();
if ($isRangeSelection(selection)) {
  const node = getSelectedNode(selection);
  const parent = node.getParent();
  const isLink = $isLinkNode(parent) || $isLinkNode(node)
}
// Tiptap
const { editor } = useCurrentEditor()
if (!editor) {
  return null
}
const isLink = editor.isActive('link')

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:

  1. components/RichTextEditor - The main, editable Tiptap instance in use when editing paragraphs, along with a menu bar component containing buttons for formatting.
  2. EditParagraph.tsx - Swap the current Textarea input with Tiptap and register the form validations.
  3. ParagraphField.tsx - Render a read-only instance of Tiptap, which keeps styling consistent
  4. editorStyles.tsx - ChakraUI resets the styles of any native HTML elements, thus we have to add these styles back.
  5. 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 using markedjs into HTML content for Tiptap to read and edit.

Current features enabled in the rich text editor:

  1. Font styles - Bold, Italic, Strikethrough
  2. Lists - ordered and unordered
  3. History - undo/redo
  4. Heading - only H2
  5. Menu buttons for all of the above
  6. Auto-links
  7. Markdown shortcuts

Breaking Changes

  • Yes - this PR contains breaking changes
    • Database changes - paragraph fields transformed from markdown string to HTML string
    • Front-end - Paragraph field changed to a rich text editor that edits and renders HTML string

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:

// RHF Controller Render
<Controller
  render={({ field, fieldState }) => (
    <RichTextEditor
      onChange={field.onChange}
      value={field.value}
      invalid={fieldState.invalid}
    />
  )}
/>

// RichTextEditor.tsx
useEffect(() => {
    if (invalid && !editor?.isFocused)
      editor?.chain().focus().scrollIntoView().run()
  }, [invalid, editor, editor?.isFocused])

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

image

Tests

Manual tests of the features above

@justynoh
Copy link
Contributor

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.mov

Spacing

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.

- Level 1
  - Level 2

should have the same spacing as

- Level 1
- Level 1

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.

image

Once these core functionality points have been ironed out, I'll start looking at the code for reviews :)

@justynoh
Copy link
Contributor

Also, tagging @staceytan1998 for design visibility.

@LeonardYam LeonardYam marked this pull request as ready for review February 14, 2024 05:52
@LeonardYam
Copy link
Contributor Author

LeonardYam commented Feb 14, 2024

Hi @justynoh, I've just looked through the comments, here are the changes made:

  1. List spacing - instead of manually applying the spacing, I reverted the list styles with all: revert, which fixes this issue.
  2. Numbered list notation - fixed by adding styles to .tiptap ol ol and .tiptap ol ol ol. Do note that we still allow infinitely nested lists and they all default to lower roman notation.
  3. Menu buttons - Removed the heading, undo/redo buttons, and downsized the rest of the buttons.
  4. Headers - removed headers from editor input/output, do note that any header tags from existing forms will be parsed as <p>.
  5. Links - unfortunately Tiptap only has built-in support for autolinks and not markdown links. Including a link button is also not as straightforward as the other buttons, and I would have to do some investigation on how to implement this properly if needed.
  6. Responsive menu bar - with the removal of 2 buttons and the downsizing of the rest, I believe the responsiveness of the menu bar is mostly fixed. I've also added a dummy link button for testing the design - let me know if there needs to be additional work on this!

@justynoh
Copy link
Contributor

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)

{
    label: 'Set link',
    icon: <RiLink />,
    onClick: (editor: Editor) => {
      const previousUrl = editor.getAttributes('link').href
      const url = window.prompt('URL', previousUrl)

      // cancelled
      if (url === null) {
        return
      }

      // empty
      if (url === '') {
        editor.chain().focus().extendMarkRange('link').unsetLink().run()

        return
      }

      // update link
      editor
        .chain()
        .focus()
        .extendMarkRange('link')
        .setLink({ href: url })
        .run()
    },
  },
  {
    label: 'Remove link',
    icon: <RiLinkUnlink />,
    onClick: (editor: Editor) => editor.chain().focus().unsetLink().run(),
  },

In this case you can see a window.prompt is used, but we definitely want something nicer than a prompt. If this snippet gives you an idea on how to implement hyperlinked text, we can explore a bit more on what implementations of link entry make sense. Let me know!

@LeonardYam
Copy link
Contributor Author

LeonardYam commented Feb 25, 2024

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 react-hook-form.

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 😊

@staceytan1998
Copy link

staceytan1998 commented Feb 29, 2024

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!!

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.

3 participants