feat(editor): improve documentation editor using Remirror #6631

ngamanda · 2022-12-04T17:03:41Z

Highlights

Improved UX for documentation editing with
- Modals to Add Links / Images
- Inline menus for quicker access to style text
- Inline link previews
@mentions: users can now tag entities by typing @. More details about the entity shown upon hovering over the mention
Resizable images: users can now resize images
- By default, uploading images will be saved as markdown (i.e. ![alt text](my-image.png)). Upon resizing, the image will be stored as HTML Markup (i.e. <img height="46" width="54" src="my-image.png" alt="alt text" />)
Table editing
- Controls to add/remove table rows/columns easily
- Tables are converted to markdown where possible. Otherwise, it will be stored as in HTML Markup. (Markdown tables cannot contain lists, headings, code blocks, or another table)

Summary of Changes

Install additional packages - @remirror/* for editor, prosemirror-autocomplete for powering inline suggestions, turndown-plugin-gfm to add gfm support for tables
Add custom buttons/controls to fit with AntD stylings
Add new Remirror DataHubMentionsExtension to contain rendering logic and to import prosemirror-autocomplete plugin
Remove sanitising of documentations located outside the editor (already handled by Remirror)
Update Jest configuration to explicitly transpile lib0 and y-protocols, keeping '^.+\\.module\\.(css|sass|scss)$' from default configurations

Checklist

The PR conforms to DataHub's Contributing Guideline (particularly Commit Message Format)
Links to related issues (if applicable)
Tests for the changes have been added/updated (if applicable)
Docs related to the changes have been added/updated (if applicable). If a new feature has been added a Usage Guide has been added for the same.
For any breaking change/potential downtime/deprecation/big changes an entry has been made in Updating DataHub

github-actions · 2022-12-04T18:05:40Z

Unit Test Results (build & test)

621 tests ±0 617 ✔️ ±0 15m 46s ⏱️ -1s
157 suites ±0     4 💤 ±0
157 files ±0     0 ❌ ±0

Results for commit 739654c. ± Comparison against base commit 2de9d3d.

♻️ This comment has been updated with latest results.

chriscollins3456

This is looking incredible! I have some comments here and there, some suggestions and some requests for things like docs and tests. However overall this is looking so good.

I pulled it down myself and am having a blast with it locally! You guys absolutely killed this. After you get back to us with my comments / suggestions let's work to get this in soon!

chriscollins3456 · 2022-12-20T18:45:10Z

datahub-web-react/craco.config.js

+    jest: {
+        configure: (jestConfig) => {
+            jestConfig.transformIgnorePatterns = [
+                'node_modules/(?!(lib0|y-protocols)).+\\.(js|jsx|mjs|cjs|ts|tsx)$',
+                '^.+\\.module\\.(css|sass|scss)$',
+            ];
+            return jestConfig;
+        },
+    },


can you add a comment as to what this is doing and what it's necessary?

chriscollins3456 · 2022-12-20T18:46:46Z

datahub-web-react/src/app/entity/shared/tabs/Documentation/DocumentationTab.tsx

    const hideLinksButton = properties?.hideLinksButton;
    const { urn, entityData } = useEntityData();
    const refetch = useRefetch();
    const description = entityData?.editableProperties?.description || entityData?.properties?.description || '';
-    const sanitizedDescription = DOMPurify.sanitize(description);


even if remirror also handles this, do you think it's a problem to sanitize here as well? just in case anything ever changes with remirror I really want to avoid any unsafe scripts or anything getting into documentation

same idea with sanitizing below before sending to the BE

Hmm, I remembered the double sanitisation did throw out some unwanted syntax in the output. Let me take a look at it again.

okay if that's the case I don't think it's a problem as long as we feel good about remirror being able to handle this for us

chriscollins3456 · 2022-12-20T21:01:04Z

datahub-web-react/src/app/entity/shared/tabs/Documentation/components/editor/Editor.tsx

+    onChange?: (md: string) => void;
+};
+
+export const Editor = forwardRef((props: EditorProps, ref) => {


nice this is my first time seeing forwardRef - nicer than having to explicitly pass in a ref as another old prop!

chriscollins3456 · 2022-12-20T21:01:23Z

datahub-web-react/src/app/entity/shared/tabs/Documentation/components/editor/Editor.tsx

+    useMount(() => {
+        manager.view.focus();
+    });


i've been waiting for something like useMount... what a time to be alive

Haha yup, react-use is a lifesaver!

chriscollins3456 · 2022-12-20T21:02:24Z

datahub-web-react/src/app/entity/shared/tabs/Documentation/components/editor/Editor.tsx

+import { MentionsComponent } from './extensions/mentions/MentionsComponent';
+
+type EditorProps = {
+    readonly?: boolean;


minor: can we change to camel case and be readOnly?

Will make the change

chriscollins3456 · 2022-12-20T21:45:11Z

...shared/tabs/Documentation/components/editor/extensions/mentions/DataHubMentionsExtension.tsx

+    handleEvents?: Handler<(action: AutocompleteAction) => boolean>;
+}
+
+class DataHubMentionsExtension extends NodeExtension<DataHubMentionsOptions> {


yeah as I mentioned in another comment, some docs here would be great as creating out own remirror extension requires a lot of special knowledge.

chriscollins3456 · 2022-12-20T21:46:31Z

...b-react/src/app/entity/shared/tabs/Documentation/components/editor/toolbar/AddLinkButton.tsx

+import { CommandButton } from './CommandButton';
+import { LinkModal } from './LinkModal';
+
+export const AddLinkButton = () => {


these are great individual toolbar components! very useful and organzied

chriscollins3456 · 2022-12-20T21:48:45Z

datahub-web-react/src/app/entity/shared/tabs/Documentation/components/editor/toolbar/Icons.tsx

+import Icon from '@ant-design/icons';
+import { CustomIconComponentProps } from '@ant-design/icons/lib/components/Icon';
+
+export const CodeIcon = (props: Partial<CustomIconComponentProps>) => (


this is awesome

chriscollins3456 · 2022-12-20T21:58:49Z

...react/src/app/entity/shared/tabs/Documentation/components/editor/toolbar/FloatingToolbar.tsx

+    overflow: hidden;
+`;
+
+export const FloatingToolbar = () => {


it looks like the floating toolbar gets cut off from the top toolbar:

Will fix this!

chriscollins3456 · 2022-12-20T22:10:24Z

datahub-web-react/src/app/entity/shared/tabs/Documentation/components/editor/Editor.tsx

+                            <Toolbar />
+                            <CodeBlockToolbar />
+                            <FloatingToolbar />
+                            <TableComponents tableCellMenuProps={{ Component: TableCellMenu }} />


is it possible to resize columns and rows of the table with your mouse? I'm trying to do it locally but not figuring out how if it is possible

Actually it's possible and it's enabled in Grab's fork. The downside is that the table will be stored in HTML rather than Markdown format. I wasn't sure if DataHub wanted that activated since you can't 'un-resize' a table (or we could also introduce a button to 'un-resize' it but I'll need more time to investigate it).

okay gotcha! yeah if it were possible to resize and store in markdown I think that would be fantastic, but if it's too much of a lift initially I think we can come back to that as a followup improvement.

chriscollins3456

This is looking so great!

Thanks so much for getting back to my comments and adding all the tests and what not. I pulled down again and tried locally and things are looking awesome :)

I have one more comment about making links target="_blank" so they open in a new tab, but otherwise I think this thing is ready to rumble!

I did notice that there's a few conflicts with master, though, so once those are resolved and we get CI to pass everything, let's get this guy merged.

chriscollins3456 · 2022-12-29T15:11:33Z

...ity/shared/tabs/Documentation/__tests__/components/editor/extensions/htmlToMarkdown.test.tsx

@@ -0,0 +1,69 @@
+import { htmlToMarkdown } from '../../../../components/editor/extensions/htmlToMarkdown';
+
+const cases = [


chriscollins3456 · 2022-12-29T15:49:19Z

...b-web-react/src/app/entity/shared/tabs/Documentation/components/editor/toolbar/LinkModal.tsx

+    handleClose: () => void;
+};
+
+export const LinkModal = (props: LinkModalProps) => {


is there a way that we can add target: "_blank" to links created in documentation so they open in a new tab?

Sure, fixed in bd3d8d2

chriscollins3456 · 2023-01-03T20:41:51Z

.github/workflows/docker-unified.yml

+        env:
+          NODE_OPTIONS: "--max-old-space-size=3072"


why is this change necessary?

Oh it's to resolve the failing build that was caused by FATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memory. in https://github.com/datahub-project/datahub/actions/runs/3804191769/jobs/6471221050. This line simply increases the memory limit allocated to node. If there are other solutions please feel free to make changes to the PR too!

it looks like this failure is still happening :( do you know if we need to put this type of change elsewhere?

another initiative we've discussed is removing the unneeded dependencies from our package.json - unsure how many things we can remove but if we can shrink our dependencies this may help solve this problem! it's something i'm looking into and will see if it helps here

Hmm, the latest build passed though: https://github.com/datahub-project/datahub/actions/runs/3805789729/jobs/6559405831. The only pipeline that's failing is https://github.com/datahub-project/datahub/actions/runs/3805789729/jobs/6559407548 which looks unrelated to the documentation, probably a flaky test (but I can't rereun it :x)

Also let me know if you need help looking into shrinking the dependencies 🗡️ !

ah so it looks like it's fixed in the Docker, Build, Scan, Test runs, but i'm still seeing the error in the logs on the general build & test / build job (https://github.com/datahub-project/datahub/actions/runs/3805789734/jobs/6521531814) - you'll have to search for it cuz that's a huge log lol

also it seems that that's timing out after 60 minutes, my hunch is that it's related to this build failing or even that something is taking much longer for some reason.

chriscollins3456 · 2023-01-12T16:47:21Z

everything is passing in CI now woo! except for these failing Monitoring tests but those are failing for all open PRs

ngamanda · 2023-01-13T01:56:42Z

Awesome, thanks for the help @chriscollins3456 !

jjoyce0510

I finally got a chance to take a deep look at this PR.

All I have to say is: WOW. The new experience looks and feels incredible - A major improvement on usability that will benefit many people (thousands of active users) in the community directly!

This PR represents an incredible piece of collaboration between Grab and the DataHub Community. I want to say thank you to @ngamanda, @chriscollins3456, and of course Harvey on behalf of the entire DataHub Team and the broader community. I look forward to continued partnership between our teams as we continue to improve DataHub.

Very happy to give this a final "LGTM" stamp.

Cheers!
John

…oject#6631) Co-authored-by: Chris Collins <[email protected]>

github-actions bot added the product PR or Issue related to the DataHub UI/UX label Dec 4, 2022

ngamanda force-pushed the remirror-documentation branch 2 times, most recently from b114d15 to 739654c Compare December 5, 2022 05:45

anshbansal added the community-contribution PR or Issue raised by member(s) of DataHub Community label Dec 6, 2022

jjoyce0510 requested a review from chriscollins3456 December 6, 2022 22:48

chriscollins3456 self-assigned this Dec 6, 2022

chriscollins3456 reviewed Dec 20, 2022

View reviewed changes

ngamanda requested a review from chriscollins3456 December 26, 2022 13:20

chriscollins3456 approved these changes Dec 29, 2022

View reviewed changes

ngamanda force-pushed the remirror-documentation branch from bd3d8d2 to 3dd5e1f Compare December 30, 2022 01:38

chriscollins3456 reviewed Jan 3, 2023

View reviewed changes

ngamanda force-pushed the remirror-documentation branch from ffb810a to 5cbf5ac Compare January 9, 2023 02:15

ngamanda added 11 commits January 9, 2023 19:31

Create editor

f3ae52d

Rename readonly to readOnly

a8b7121

Add documentation

2632e9f

Add tests for htmlToMarkdown

1d7d6f7

Fix floating toolbar getting cut off

a755b5e

Add tests for markdownToHtml

7b33748

Rename _suggestions to suggestions

5cc7d04

Use remirror across DataHub

cee082d

Use content instead of controlledContent

58a1ad4

Set links target to be _blank

48c1226

Increase node max-old-space-size to 3GB

a255acc

ngamanda force-pushed the remirror-documentation branch from 5cbf5ac to a255acc Compare January 9, 2023 13:13

chriscollins3456 added 4 commits January 11, 2023 12:16

extend timeouts for long running frontend tests

9864842

final test update

808e655

fix slow tests causing build to fail

fe13b60

disable lint on unused props for Editor mock

071081a

jjoyce0510 approved these changes Jan 17, 2023

View reviewed changes

chriscollins3456 merged commit 087b169 into datahub-project:master Jan 17, 2023

shirshanka pushed a commit to shirshanka/datahub that referenced this pull request Jan 18, 2023

feat(editor): improve documentation editor using Remirror (datahub-pr…

f203905

…oject#6631) Co-authored-by: Chris Collins <[email protected]>

ericyomi pushed a commit to ericyomi/datahub that referenced this pull request Jan 18, 2023

feat(editor): improve documentation editor using Remirror (datahub-pr…

d227b6c

…oject#6631) Co-authored-by: Chris Collins <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(editor): improve documentation editor using Remirror #6631

feat(editor): improve documentation editor using Remirror #6631

ngamanda commented Dec 4, 2022

github-actions bot commented Dec 4, 2022 •

edited

Loading

chriscollins3456 left a comment

chriscollins3456 Dec 20, 2022

chriscollins3456 Dec 20, 2022

chriscollins3456 Dec 20, 2022

ngamanda Dec 23, 2022

chriscollins3456 Dec 27, 2022

chriscollins3456 Dec 20, 2022

chriscollins3456 Dec 20, 2022

ngamanda Dec 23, 2022

chriscollins3456 Dec 20, 2022

ngamanda Dec 23, 2022

chriscollins3456 Dec 20, 2022

chriscollins3456 Dec 20, 2022

chriscollins3456 Dec 20, 2022

chriscollins3456 Dec 20, 2022

ngamanda Dec 23, 2022

chriscollins3456 Dec 20, 2022

ngamanda Dec 23, 2022

chriscollins3456 Dec 27, 2022

chriscollins3456 left a comment

chriscollins3456 Dec 29, 2022

chriscollins3456 Dec 29, 2022

ngamanda Dec 30, 2022

chriscollins3456 Jan 3, 2023

ngamanda Jan 4, 2023

chriscollins3456 Jan 5, 2023

chriscollins3456 Jan 5, 2023

ngamanda Jan 6, 2023

ngamanda Jan 6, 2023

chriscollins3456 Jan 6, 2023

chriscollins3456 commented Jan 12, 2023

ngamanda commented Jan 13, 2023

jjoyce0510 left a comment

		@@ -0,0 +1,69 @@
		import { htmlToMarkdown } from '../../../../components/editor/extensions/htmlToMarkdown';

		const cases = [

feat(editor): improve documentation editor using Remirror #6631

feat(editor): improve documentation editor using Remirror #6631

Conversation

ngamanda commented Dec 4, 2022

Highlights

Summary of Changes

Checklist

github-actions bot commented Dec 4, 2022 • edited Loading

Unit Test Results (build & test)

chriscollins3456 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriscollins3456 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriscollins3456 commented Jan 12, 2023

ngamanda commented Jan 13, 2023

jjoyce0510 left a comment

Choose a reason for hiding this comment

github-actions bot commented Dec 4, 2022 •

edited

Loading