Add NestedElementField #324

rhystmills · 2023-12-18T15:15:19Z

Co-authored with @davidfurey and @jonathonherbert - first few commits were David's but I unintentionally rebased his name out of the commit history.

What does this change?

This PR adds the nested element field. This field is a rich text editor that is itself able to contain elements. This will enable us to build larger, 'structural' elements within Composer that can contain elements like images and videos.

There is some further work to do in future PRs:

Add some helper functions to provide a content expression for a nested element field that disallows certain elements (for example, we probably don't want elements that contain nested element fields within other nested element fields).
There is a known bug in repeater fields involving applying Decorations . Trying to insert an element (e.g. via pasting) into any node other than the initial one in a repeater field now works, but the Decorations for notes and Typerighter matches within nestedElements aren't currently working.
Make sure that the element controls are usable within the field. (e.g. move, delete, select)

This PR could do with some tests for nested element fields - I'll add some when I'm back from holiday.

How to test

Run the demo app for this branch with yarn start.
Add the 'Alt Style' element.
Can you past other elements into the 'Content' field? (You should be able to).

You may also want to test this PR in Composer via yalc with: https://github.com/guardian/flexible-content/pull/4492

as we expect the child editor to reflect parent's behaviour

as its content will be a part of the document proper

…eateNestedFieldView from package

…ter focus state

…f blur for use in main prosemirror editor (in program consuming prosemirror-elements)

meant eslint was unhappy with the naming convention

davidfurey · 2023-12-19T09:50:38Z

As discussed before, I'm unsure on the best name for this. Perhaps ElementsField?

rhystmills · 2024-01-04T10:10:33Z

As discussed before, I'm unsure on the best name for this. Perhaps ElementsField?

I went with nestedElement rather than just nested in the end - going for just 'element' or 'elements' felt confusing in some places in the codebase. Does that seem alright to you?

src/editorial-source-components/Editor.ts

Fweddi · 2024-01-08T16:19:24Z

src/plugin/fieldViews/NestedElementFieldView.ts

+      getPos,
+      offset,
+      decorations,
+      // Allow plugins without a plugin key, but exclude those that are explicitly blocked


Suggested change

// Allow plugins without a plugin key, but exclude those that are explicitly blocked

Disallow plugins which have a plugin key, or are explicitly blocked.

I think this is clearer, but not sure, feel free to ignore.

I think my initial comment is not great here (mostly duplicating the code), I think the useful information would actually be:
"Keys are an optional unique identifier for plugins; we should expect to encounter plugins without keys."

Currently more plugins exist without keys than with keys in Composer, and they are likely to be doing important work. That's why I've gone with a blocklist rather than an allowlist.

@rhystmills and I paired on this briefly to clarify the code. Worth noting my suggestion above was incorrect!

Fweddi · 2024-01-08T16:27:46Z

demo/index.ts

+    /*
+     * We must allow 2 text elements to exists side by side
+     * so that when we delete an element we can then join it.
+     */


Not entirely sure I understand this. Is this just a general bug that's being fixed here, or is it related to the nested elements?

Also, the comment here mentions two text elements but the content field is talking in terms of blocks, just thought I would flag in case that's unintended.

Whoops - I think I unintentionally committed that after borrowing the base node spec from Composer's block editor.

Definitely not relevant to the PR!

Fweddi · 2024-01-08T16:31:20Z

src/plugin/__tests__/nodeSpec.spec.ts

+        const nodeSpec = getNodeSpecForField(
+          "exampleElement",
+          "exampleNestedElementField",
+          createNestedElementField({ content: "element*" })


Elsewhere this is defined with content as block*, is this an inconsistency?

I would have thought that a nested element would contain element*, but I suppose the nested elements are also nested in blocks?

I've said nested too many times... 🪆

🪆

The top level block* content expression is specific to the demo in prosemirror-elements - element+/element* is closer to what we'll see in flexible-content where the top level editor has an element+ content expression. At the moment there's a bit of a compromise to allow Composer and the demo to both work.

It would be good to align the demo more closely with what we see in flexible-content so we don't have this discrepancy. I could add that as a follow-up card to follow this one, or we could fix in this PR?

The demo could do with some love in general, e.g. having far fewer elements.

That makes sense and sounds good to me. The difference between demo and Composer does confuse me from time to time (the Alt Styles element in this PR for example threw me).

Fweddi · 2024-01-08T16:35:33Z

src/plugin/field.ts

@@ -55,6 +55,8 @@ export const getFieldsFromNode = <FDesc extends FieldDescriptions<string>>({
    const fieldDescription = fieldDescriptions[fieldName];
    // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- strictly, we should check.
    if (!fieldDescription) {
+      console.log({ fieldNode, fieldDescriptions, fieldName });


Might not be needed?

Fweddi · 2024-01-08T16:43:19Z

src/plugin/helpers/__tests__/fixtures.ts

 import { createRepeaterField } from "../../fieldViews/RepeaterFieldView";
 import { createTextField } from "../../fieldViews/TextFieldView";
 import { RepeaterFieldMapIDKey } from "../constants";
 import { createEditorWithElements, createNoopElement } from "../test";
 import { maxLength, required } from "../validation";

 export const elements = {
+  nestedElement: createNoopElement({
+    content: createTextField({


Interested in this as it seems to be doing something similar but different to the nestedElementField below. Also it's called nestedElement, but seems to be a text no-op element with a text field? Is that what a nestedElement is, secretly?

Unclear naming on my part I think - this is essentially a test element, separate from the 'example' element used in the tests, to put in the nestedElementField in the tests.

I think exampleElementToNest might be better. Have now renamed.

Fweddi · 2024-01-08T17:31:19Z

I can't copy / cut an element that is nested - I can select it, but the copy and cut functions don't seem to work. I don't know if this is related to the repeater fields bug, or is something separate.

Fweddi · 2024-01-08T17:31:39Z

I also can't reorder nested elements using the single arrows (I can using the double arrows).

Fweddi · 2024-01-08T17:36:17Z

This might be an issue with the demo tool, but if I delete all the text in a Nested Element Field, and there's another element nested, I can't edit the field (or can't click into it to type / add elements).

Fweddi · 2024-01-08T17:37:25Z

I also can't reorder nested elements using the single arrows (I can using the double arrows).

I see you've mentioned this as work for a follow-up PR. 👍

Fweddi

A great, hefty piece of work that moves us closer to our OKR, amazing stuff.

There are a few bugs and edge cases, but I think it makes sense to fix forwards and address these in smaller PRs. It would be good too to document some of these changes in a README somewhere; at a glance I found myself getting confused between Nested and Repeater fields, but the code is also fairly self-explanatory.

Co-authored-by: Tom Richards <tom.richards@guardian.co.uk>

…ocklist with allowedPlugin list Co-authored-by: Freddie Preece <40991816+fweddi@users.noreply.github.com>

…w` since it's just a vehicle for other elements so doesn't need to worry about decorations itself Co-authored-by: Rhys Mills <34686302+rhystmills@users.noreply.github.com>

Co-authored-by: Freddie Preece <40991816+Fweddi@users.noreply.github.com>

…ed to NestedField for decoration management

Fweddi · 2024-01-16T12:01:04Z

src/plugin/fieldViews/NestedElementFieldView.ts

+      // We use a disallow list (rather than an allow list) because we expect most plugins to work,
+      // and only want to disallow those that cause problems.


We'll need to remove this comment now we've changed to an allowlist.

Fweddi

Subsequent nodes of the repeater field now work as expected 👍

There are still some small issues around decorations but I think this puts us in a good place to merge.

rhystmills and others added 12 commits December 1, 2023 12:18

wip but seems to work

710979e

Use plugins from parent editor

3082972

as we expect the child editor to reflect parent's behaviour

No need to restrict field height

8bee728

as its content will be a part of the document proper

Position menus and action buttons properly relative to nested element

65d9a0b

Enable editor to serialise and deserialise nestedFields and export cr…

f510dc4

…eateNestedFieldView from package

Sort styling issues with nested fields and alt styles

52ef2f4

Make demo happy and use passed in content expression for nested field

42d7ca6

Make typescript happy

2248898

Update content expression for nested field to require one element, al…

8838ef2

…ter focus state

Allow disallowance of plugins and send event on nested editor focus o…

6b23698

…f blur for use in main prosemirror editor (in program consuming prosemirror-elements)

Appease linter

e9958a6

Rename NestedField to NestedElementField

a190cff

rhystmills changed the title ~~wip but seems to work~~ Add NestedElementField Dec 18, 2023

rhystmills added 2 commits December 18, 2023 15:59

Rename enum from NESTED_ELEMENTS to NESTED because the underscore

e72c0e2

meant eslint was unhappy with the naming convention

Fix alt styles and unit tests

bbd063c

rhystmills added 2 commits January 3, 2024 15:24

Update field tests to include nested element fields

8a5fe65

Add nodeSpec tests for nestedField

6d27c2f

rhystmills added 3 commits January 4, 2024 10:33

Explain what the custom events do

c7e01f5

Add element tests for element containing nestedElementField

28227b6

Spark delight for linter

bf66ef4

rhystmills marked this pull request as ready for review January 4, 2024 12:15

rhystmills requested a review from a team as a code owner January 4, 2024 12:15

Fweddi reviewed Jan 8, 2024

View reviewed changes

src/editorial-source-components/Editor.ts Outdated Show resolved Hide resolved

Fweddi reviewed Jan 8, 2024

View reviewed changes

rhystmills added 5 commits January 9, 2024 09:31

Rename nestedElement in tests and fixtures to remove ambiguity

aa92d28

Remove irrelevant comment

bd8a27b

Remove unneeded comment

b4df293

Provide more useful information in comment

0094624

Appease linter

c248b37

Fweddi approved these changes Jan 9, 2024

View reviewed changes

Make plugin filtering code a bit more readable

6005aa2

twrichards force-pushed the nest-elements branch from 4a9e3ed to 33a5f04 Compare January 10, 2024 09:11

Add failing and succeeding cases for nested element in repeater node

74a837d

Co-authored-by: Tom Richards <tom.richards@guardian.co.uk>

twrichards force-pushed the nest-elements branch from 33a5f04 to 74a837d Compare January 10, 2024 17:05

Fweddi force-pushed the nest-elements branch from 83675dc to 7590120 Compare January 12, 2024 15:33

rhystmills and others added 3 commits January 12, 2024 15:34

Use repeater field in alt style element, replace disallowed plugin bl…

fd77770

…ocklist with allowedPlugin list Co-authored-by: Freddie Preece <40991816+fweddi@users.noreply.github.com>

Override applyDecorationsFromOuterEditor for `NestedElementFieldVie…

dee502e

…w` since it's just a vehicle for other elements so doesn't need to worry about decorations itself Co-authored-by: Rhys Mills <34686302+rhystmills@users.noreply.github.com>

WIP attempt to get repeater working via some unsafe type casting

a077518

Fweddi force-pushed the nest-elements branch from 7590120 to a077518 Compare January 12, 2024 15:41

rhystmills and others added 3 commits January 15, 2024 12:19

Update src/editorial-source-components/Editor.ts

deae41a

Co-authored-by: Freddie Preece <40991816+Fweddi@users.noreply.github.com>

Use safe typeguards for decoration type differentiation, remove chang…

27ffbe6

…ed to NestedField for decoration management

Make linter happy

446188c

Fweddi reviewed Jan 16, 2024

View reviewed changes

Describe allowList and DecorationSet/DecorationGroup

b0d8e0e

rhystmills force-pushed the nest-elements branch from ee2c43b to b0d8e0e Compare January 16, 2024 13:22

Fweddi self-requested a review January 16, 2024 14:08

Fweddi approved these changes Jan 16, 2024

View reviewed changes

rhystmills merged commit 3cc94dc into main Jan 16, 2024
3 checks passed

rhystmills deleted the nest-elements branch January 16, 2024 15:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add NestedElementField #324

Add NestedElementField #324

rhystmills commented Dec 18, 2023 •

edited

Loading

davidfurey commented Dec 19, 2023

rhystmills commented Jan 4, 2024

Fweddi Jan 8, 2024

Fweddi Jan 8, 2024

rhystmills Jan 9, 2024

rhystmills Jan 9, 2024

Fweddi Jan 9, 2024 •

edited

Loading

Fweddi Jan 8, 2024

rhystmills Jan 9, 2024 •

edited

Loading

Fweddi Jan 8, 2024

rhystmills Jan 9, 2024 •

edited

Loading

Fweddi Jan 9, 2024

Fweddi Jan 8, 2024

rhystmills Jan 9, 2024

Fweddi Jan 8, 2024 •

edited

Loading

rhystmills Jan 9, 2024 •

edited

Loading

Fweddi commented Jan 8, 2024

Fweddi commented Jan 8, 2024

Fweddi commented Jan 8, 2024

Fweddi commented Jan 8, 2024

Fweddi left a comment

Fweddi Jan 16, 2024

Fweddi left a comment

	// Allow plugins without a plugin key, but exclude those that are explicitly blocked
	Disallow plugins which have a plugin key, or are explicitly blocked.

		// We use a disallow list (rather than an allow list) because we expect most plugins to work,
		// and only want to disallow those that cause problems.

Add NestedElementField #324

Add NestedElementField #324

Conversation

rhystmills commented Dec 18, 2023 • edited Loading

What does this change?

How to test

davidfurey commented Dec 19, 2023

rhystmills commented Jan 4, 2024

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

Fweddi Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhystmills Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhystmills Jan 9, 2024 • edited Loading

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

Fweddi Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

rhystmills Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

Fweddi commented Jan 8, 2024

Fweddi commented Jan 8, 2024

Fweddi commented Jan 8, 2024

Fweddi commented Jan 8, 2024

Fweddi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fweddi left a comment

Choose a reason for hiding this comment

rhystmills commented Dec 18, 2023 •

edited

Loading

Fweddi Jan 9, 2024 •

edited

Loading

rhystmills Jan 9, 2024 •

edited

Loading

rhystmills Jan 9, 2024 •

edited

Loading

Fweddi Jan 8, 2024 •

edited

Loading

rhystmills Jan 9, 2024 •

edited

Loading