Get minimum ProseMirror editor working #1

rhystmills · 2023-10-19T15:54:43Z

What does this change?

This gets a basic ProseMirror editor working. The aim is to create a general purpose editor that satisfied the requirements of:

Primarily:
Secondly:
- media-atom-maker

media-atom-maker is a lower priority because it's already been moved to ProseMirror from Scribe (so there's no Scribe-based vulnerability), but we want to make this package compatible with media-atom-maker's requirements so it can be ported over in the future, so that the editors in the four repositories can all be managed via this one package.

This branch has been tested locally in tagmanager via yalc. The package has not yet been deployed to npm.

Things this PR doesn't have, that we will want to add in future PRs:

A means of testing changes from this package itself, for example a demo-app that includes the React editor component. Currently this package only builds a component to be used by other packages.
A means of deploying to an npm package.

Currently, vanilla css styles are included in the built dist and need to be imported in the target package. Is this a good setup? (A google chat thread led to recommendations against emotion and I wanted to keep this package as un-opinionated as possible).

How to test

Publish this package locally using yarn yalc from the root of the project
Use this branch of tagmanager locally.
Add that package to tagmanager using yalc add @guardian/prosemirror-editor
Run tagmanager
In tagmanager, go to 'Create a new tag' and pick an option from the dropdown.
Click on an existing tag from the home page table and inspect the rich text editor element of the Description/Profile field. Do the classes include 'prosemirror'? (They should).
Open the network tab, clear the requests. Then edit the tag's rich text, and save the changes. A POST request should go to the url similar to https://tagmanager.local.dev-gutools.co.uk/api/tag/71301. Does the description sent in that request look valid? Is the bold text serialised as  tags and the italic text as  tags? (It should). Are new paragraphs serialised as new  tags? (They should).

Co-authored-by: Jonathon Herbert <jonathon.herbert@guardian.co.uk>

…rm to legacy and markup

davidfurey

Epic PR! I've made a few suggestions, and haven't yet run through your test steps.

davidfurey · 2023-10-25T10:36:45Z

LICENSE

-                                 Apache License
-                           Version 2.0, January 2004
-                        http://www.apache.org/licenses/
+Copyright 2023 Guardian News and Media


Where did this license come from?

MIT license from: https://opensource.org/license/mit/

Great. Surprised that it isn't normal for us to state that, but happy for us to stick with our convention

Looks like we do in some repos, I'll mention it for the convenience of others viewing the license.

davidfurey · 2023-10-25T10:37:53Z

README.md

+It is also designed to meet the features required by the rich text editor in [media-atom-maker](https://github.com/guardian/media-atom-maker/blob/3391bfd82ad27848dc9e06a08628320ce481006e/public/video-ui/src/components/FormFields/RichTextEditor.tsx#L2).
+
+It aims to provide:
+- A `RichTextEditor` serialising to one-to-many <p> tags containing the markip below - supporting only the requirements of the above editors, at least until we have further use cases. Specifically, from the first three editors:


I think you might need to escape  to make this render correctly. Also, do you really mean  here?

Typo:markip

Whoops - yes it should be  but not be markip.

davidfurey · 2023-10-25T10:39:28Z

README.md

+  - unlinking
+  - strikethrough
+  - superscript and subscript
+  `media-atom-maker` adds some more requirements:


Probably need a - here

davidfurey · 2023-10-25T10:41:36Z

package.json

+{
+  "name": "@guardian/prosemirror-editor",
+  "version": "0.1.0",
+  "description": "Providing implementations of ProseMirror for use in our tools.",


our -> the Guardian ?

davidfurey · 2023-10-25T11:11:27Z

src/ts/RichTextEditor.tsx

+  shouldAcceptCopiedText: boolean;
+}
+
+export const RichTextEditor = ({ value, onUpdate, config, label, shouldAcceptCopiedText = false }: RichTextEditorProps) => {


Why is a default value required for shouldAcceptCopiedText, which is a required field?

Wise, optional now

davidfurey · 2023-10-25T11:46:47Z

src/ts/richtext/config.ts

+  allowedMarks: string[];
+};
+
+type FlatTextMark = "strong" | "strike" | "em";


You could avoid having to write these out again on line 27 with:

const flatTextMarks = ["strong", "strike", "em"] as const type FlatTextMark = typeof flatTextMarks

I did start with this - but I had trouble getting Typescript happy with partial variants of the flatTextMarks - it should be valid to have a customFlatTextConfig that only supports 'strong', for example. A greater Typescripter might find better way to do that than what I've ended up with - possibly similar to the above.

davidfurey · 2023-10-25T11:59:58Z

src/ts/richtext/createSchema.ts

+  allowedNodes: string[]
+): OrderedMap<NodeSpec> => {
+  let map = OrderedMap.from<NodeSpec>({});
+  nodes.forEach((key, node) => {


Isn't it a shame that OrderedMap doesn't have filter?

You could write your own and avoid the duplicated lines filterNodes and filterMarks. But for 2 uses, possibly not worth the complexity

davidfurey · 2023-10-25T12:07:06Z

src/ts/richtext/menu.ts

+import { isInNode, toggleBulletListCommand } from "./utils/listsHelpers";
+import { linkItemCommand, unlinkItemCommand } from "./utils/command-helpers";
+
+export const cmdItem = (cmd: Command, options: Partial<MenuItemSpec>) => {


To avoid having to cast options.title as string:

Suggested change

export const cmdItem = (cmd: Command, options: Partial<MenuItemSpec>) => {

export const cmdItem = (cmd: Command, options: Partial<MenuItemSpec> & { title: string }) => {

(Applies multiple times in this file)

Delightful, implemented now.

davidfurey · 2023-10-25T12:12:45Z

src/ts/richtext/setup.ts

+        // to format the outputHtml as an html string rather than a document fragment, we are creating a temporary div, adding it as a child, then using innerHTML which returns an html string
+        const tmp = document.createElement('div');
+        tmp.appendChild(outputHtml);
+        if (onChange) {


Is any of the work on line 45-49 valuable if onChange is undefined? Also, the types imply that onChange cannot be undefined?

Yes, good point. Not sure how that made it's way in - think the code may be borrowed from my prosemirror implementation in media-atom-maker and can't remember why past me wrote it. I'll remove the if condition.

davidfurey · 2023-10-25T12:37:47Z

src/ts/richtext/utils/richTextHelpers.ts

+};
+
+export const isTooLong = (value: string, maxWordLength: number): boolean => {
+  const wordLength = getWords(value).reduce((length, word) => {


I appreciate that this is copy and pasted from elsewhere, but as far as I can see this calculates the total number of characters in all of the words in value rather than just a count of the number of words in value

Yes, looks like it - it's a bit of validation from media-atom-maker most recently updated in guardian/media-atom-maker#1127 . Naming could be better - perhaps would be something like tooManyCharactersInAllWords would be an improvement.

It looks like there's an assumption based into the original logic that spaces aren't including in Youtube's video title character limit, which might be wrong. Seems like it would be an odd restriction.

No explanation of the odd logic in the original PR. I'd be inclined to go for a simple .length on the string value.

I'm removing these functions for now as they're currently unused -when length validation is added I'll make it simpler.

Great. I suspect there is an error in the original PR rather than the logic being intentionally odd. I'll open a PR in media-atom-maker to fix

Co-authored-by: David Furey <david@furey.me.uk>

davidfurey

lgtm

rhystmills and others added 11 commits September 8, 2023 14:53

Add .gitignore

d75bc1a

Update to MIT license

38e111e

Initial repo setup

8112840

Make react happy

e004108

Update tsconfig

3d962a7

Improve config

5454e98

Prosemirror editor -not working

331e168

Get react components exporting properly

2768c4e

Add readable labels

22cdcc0

Co-authored-by: Jonathon Herbert <jonathon.herbert@guardian.co.uk>

Include bundled styles

ae301b7

Make link prompt less annoying, improve styles, add helper to transfo…

b8c0ba0

…rm to legacy and markup

rhystmills mentioned this pull request Oct 20, 2023

Bump React to version 16.14.0 guardian/tagmanager#501

Merged

rhystmills added 2 commits October 20, 2023 12:42

Update readme with project description

8fca5f3

include more context in readme

7c10ae4

rhystmills mentioned this pull request Oct 23, 2023

Bump react to 16 guardian/atom-workshop#344

Merged

Remove unusued Editor component

627e14f

davidfurey reviewed Oct 25, 2023

View reviewed changes

rhystmills and others added 9 commits October 25, 2023 14:23

Fix some markdown problems

a9d5432

Make shouldAcceptCopiedText false

c2775cc

Use positives over negatives in if condition

211ba84

Co-authored-by: David Furey <david@furey.me.uk>

Consistently order node and mark values

90c0709

Remove need for casting title as string

d2851e7

Remove if condition that should be redundant based on types

4351b9c

Remove unused functions

ec3df9a

Namedrop org

879b12c

Mention in license that it's the MIT license

d8bc024

davidfurey approved these changes Oct 25, 2023

View reviewed changes

rhystmills merged commit 6ba81cf into main Oct 25, 2023
1 check passed

jonathonherbert deleted the rm/initialise-repo branch November 14, 2023 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get minimum ProseMirror editor working #1

Get minimum ProseMirror editor working #1

rhystmills commented Oct 19, 2023 •

edited

Loading

davidfurey left a comment

davidfurey Oct 25, 2023

rhystmills Oct 25, 2023

davidfurey Oct 25, 2023

rhystmills Oct 25, 2023

davidfurey Oct 25, 2023

rhystmills Oct 25, 2023

davidfurey Oct 25, 2023

davidfurey Oct 25, 2023

davidfurey Oct 25, 2023

rhystmills Oct 25, 2023

davidfurey Oct 25, 2023

rhystmills Oct 25, 2023

davidfurey Oct 25, 2023

davidfurey Oct 25, 2023

rhystmills Oct 25, 2023

davidfurey Oct 25, 2023

rhystmills Oct 25, 2023 •

edited

Loading

davidfurey Oct 25, 2023

rhystmills Oct 25, 2023 •

edited

Loading

rhystmills Oct 25, 2023 •

edited

Loading

rhystmills Oct 25, 2023

davidfurey Oct 25, 2023

davidfurey left a comment

	export const cmdItem = (cmd: Command, options: Partial<MenuItemSpec>) => {
	export const cmdItem = (cmd: Command, options: Partial<MenuItemSpec> & { title: string }) => {

Get minimum ProseMirror editor working #1

Get minimum ProseMirror editor working #1

Conversation

rhystmills commented Oct 19, 2023 • edited Loading

What does this change?

How to test

davidfurey 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

rhystmills Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhystmills Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

rhystmills Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfurey left a comment

Choose a reason for hiding this comment

rhystmills commented Oct 19, 2023 •

edited

Loading

rhystmills Oct 25, 2023 •

edited

Loading

rhystmills Oct 25, 2023 •

edited

Loading

rhystmills Oct 25, 2023 •

edited

Loading