SPIKE Identify opportunities to refactor InsertMediaModal

[maxime-rainville]

While working on LinkField, we identified several weakness in the implementation of InserMediaModal:

• It doesn't accommodate alternative/custom use case
• It doesn't integrate well wit the native behaviour of FormSchema
• It doesn't give you clear guidance on what happens when the user is done editing.

More broadly, the implementation is dated and the code base is not clean.

Goal

We want to identify approaches to make InsertMediaModal more reliable and flexible so it can accommodate more use cases.

We also want to indentify possible refactor opportunities to bring it up to modern React standards.

Improvement should be spilt between those:

• that can be done in CMS 5 and
• those that will require breaking changes and will need to wait for CMS 6

Timebox

• 2 days

Possible follow up work this could unlock

• LinkField link creation
• Custom meta data management when using files in specific context
• More versatile way to customise the Image, File and Folder forms in CMS 6.

Pull requests

• https://github.com/silverstripe/silverstripe-admin/pull/1631

[maxime-rainville]

The problems

Here are some key problems at first glance:

• InsertMediaModal use the FormBuilderModal for the modal part rather than for form building.
• InsertMediaModal has some convoluted logic for controlling the schema URL that gets passed to AssetAdmin. All of that logic is pointless because AssetAdmin regenerate the schema URL any way.
• Editor is where chunk of the magic need to happen.
  • The EditorHeader and PreviewImageField are implemented in such a way that they are technically FormFields in various file related form schema.
  • This is causing problem because we have to resort to all sort of crazy things to high jack their behaviour to do what they are meant to.
  • They are also completely disconnected from the form schema they are tied to.
  • When I created the logic so you could edit files directly in the InsertMediaModal, I set it up so the form you are currently editing would be tracked in a redux and you could stack what form you are editing and recall the data you stored in the previous form. That's way overkill and in practice, you'll only have the Edit file details form.
• Theres's some concerns bleeding between AssetAdmin and Editor
  • AssetAdmin::handleSubmitEditor has a lot of logic to control what should happen when the editor is done.

General approach

• https://github.com/silverstripe/silverstripe-admin/pull/1631
• Allow whatever is calling InsertMediaModal to provide its own custom FormSchema component.
  • That custom form schema logic will be passed down to AssetAdmin and eventually to the Editor
  • The Editor will call that custom form schema component with the details of the file currently being edited who which will be used to build the form schema URL.
  • Because the form schema component will exist independently from the Editor, it will be able to decide for itself what happens when a specific action is performed.
• Simplify the "Edit Detail file" logic to remove all the redux madness.
  • When a custom form schema component is provided to the editor, that will be displayed.
  • When the user decides to edit the file, we'll simply hide the custom form schema component, with CSS. This save us the problem of having to recall the value the editor has put in there, since the form won't be unloaded. The regular edit file form will be loaded in a separate FormBuilder instance.
  • If no custom FormSchema component is provided, we just go straight to rendering the regular "edit file" form.
• Update the Editor so it is directly in charge of rendering its Header and the preview.
  • PreviewImageField will need to be deprecated.
  • All the references to AssetEditorHeaderFieldGroup in the FormFactories classes can be remove/deprecated.
• Where practical, I think we should aim to re-factor class components to be functional components instead.

Breaking this into issues

To minimise complexity, we should split all the work in individual cards that build on top of each other. We could work on some of those in parallel. But I think some of those changes could be quite convoluted, so splitting them off will simplify peer review.

I suggest we create the following issues:

• Refactor FormBuilderModal and split up the Modal into its own component
• Ripe out PreviewImageField and EditorHeader to be their own component and remove them from FormSchema factories in PHP.
• Refactor the "Edit file in modal" feature to be simpler and use two FormBuilder component instead of convoluted Redux logic.
• Update the InsertMediaModal/AssetAdmin/Editor intersection so a FormSchema component can be passed on the chain to perform custom file operation.
• Update LinkField FileLink to use InsertMediaModal. (Maybe we do this simultaneously to the previous one to validate usage)
• Update current usage of InsertMediaModal to use the new approach. (Optional)
• Document how to create custom InsertMediaModal.

[GuySartorelli]

@maxime-rainville Can you please link to the issues in your "Breaking this into issues" section? Or... if those issues don't exist, can you please make it clearer that you are suggesting we make new issues but that they haven't been created yet?

[maxime-rainville]

I updated the comment to reflect that the cards haven't been created yet. Presuming people don't find this split controversial, I can create the cards.

[GuySartorelli]

The problems

Editor is where chunk of the magic need to happen.

Please explain what this component is, how it's used, and why this is where "magic" needs to happen.... and what that magic is. The points that are indented below this one don't immediately relate to this one in a way that I understand without essentially taking a deep dive into this myself - the point of the spike is that you do the deep dive for the rest of us :p

This is causing problem because we have to resort to all sort of crazy things to high jack their behaviour to do what they are meant to.

At a glance, that doesn't look crazy. Please go into more detail about what the problem actually is in high-level terms, and what an appropriate solution might look like.

They are also completely disconnected from the form schema they are tied to.

Please explain why this is a problem. Just reading this, it sounds like there's good decoupling? But I assume that's not what you meant?

General approach

Anything about the PR I'll mention in a PR review.

One detail that's missing here is how much, if any of this, can be done without breaking changes? Is this all CMS 6 work? Is there some of this that would be in CMS 5?

Allow whatever is calling InsertMediaModal to provide its own custom FormSchema component.

What is a FormSchema component? Is that something that already exists, or would it be new? I can't find a component with that name in admin nor asset-admin.

If it's new, does it have uses beyond asset admin? What exactly is its purpose? Does it have any linkages up to PHP code e.g. getting a PHP Form object and converting it into the appropriate react components?

When the user decides to edit the file, we'll simply hide the custom form schema component, with CSS. This save us the problem of having to recall the value the editor has put in there, since the form won't be unloaded. The regular edit file form will be loaded in a separate FormBuilder instance.

If the form is in a modal, this sounds sub-optimal to me. I'd much rather that modals are only rendered when they're needed. Having random forms hidden away with CSS means we have to be more careful about people mucking about unhiding them and doing things we don't want them to do (and is just untidy, besides)

Update the Editor so it is directly in charge of rendering its Header and the preview.

Is there a use case where someone will want to customise these, which this change might make more difficult?

PreviewImageField will need to be deprecated.

Can that field currently be used in other contexts? If so, deprecating it would mean we're removing it which means those use cases won't be possible anymore...

All the references to AssetEditorHeaderFieldGroup in the FormFactories classes can be remove/deprecated.

Can developers currently add fields/actions to that field group? If so, will this refactor provide a new way to do so?

Where practical, I think we should aim to re-factor class components to be functional components instead.

That honestly sounds like a waste of time. What benefit does this give us? The code works as is, so what does a functional component do that a class component doesn't? Is it more performant or provide some other clear benefit that justifies this other than just being the current convention that React wants us to follow?

Breaking this into issues

Assuming all of the suggested solutions are sensible, breaking them into separate issues seems sensible. Though perhaps "Document how to create custom InsertMediaModal." should include documenting why you might want to do that.

Update LinkField FileLink to use InsertMediaModal.

Why? What benefit does that provide over the current link field modal? Surely a consistent link editing experience is better than having a completely different modal for each link type?

Update current usage of InsertMediaModal to use the new approach

That doesn't seem optional to me? Isn't the whole point that we clean up the mess that's there?

[maxime-rainville]

Behold, the <Editor>

what this component is

The Editor is the component that renders a Form in the AssetAdmin component. It uses FormBuilderLoaderComponent to load a form schema.

how it's used

It's only directly used by AssetAdmin. It received a bunch of info about the currently selected file in the editor and some random bit of context (is AssetAdmin being served from the assets section or inside the modal, what's the action currently being performed by the user, etc.)

From there, it will decide how to generate the SchemaURL to get the correct form schema.

It will also handles each one of those possible form schema in different ways and high jack the natural form schema interaction for its own purpose.

why this is where "magic" needs to happen

Basically, the editor is trying to do everything for every file interaction you might have. If you are doing work on the editor you have to be aware of how a file gets updated, how to add a file to the campaign, how to add a file to a WYSIWYG, how to attach a file the Upload field.

In short, it's just too much complexity to manage in one spot.

crazy things to high jack their behaviour

This point was just there to highlight one specific aspect of the craziness.

In that very specific case, we are trying to control the Header of the Editor.

Some times, we need to

• display a back button or
• a Details button or
• both or
• none.

This should be as simple as sticking a simple component into Editor, right? NO, because the Editor Header is built into the FormSchema of every file form. To actually interact with it, you have to provide FormBuilderLoader with a createFn method. That method gets called for every field created via Redux form.

Our custom createFn method, look at the name of the field it receives and plainly instantiate the component all the time ... except for AssetEditorHeaderFieldGroup where it creates a brand new component to wrap around the actual component redux form wants to create.

This entire 60 lines of code could easily be replaced with 2-3 lines of code to wire a straight forward component.

They are also completely disconnected from the form schema they are tied to.

From the point of view of the file form schema in PHP:

• the EditorHeader is a just a FieldGroup with HeaderField in it
• the PreviewImageField is structured as a form field, but it doesn't allow you to save anything in the form schema.

Those are disconnected from the form schema because they don't represent any value that could be saved. They are only there so the Editor can attach a bunch of functionality on them that exists outside of the form schema or FormBuilderLoader.

By moving them outside the form schema, we can simplify the form schema and the Editor at the same time.

One detail that's missing here is how much, if any of this, can be done without breaking changes?

The PHP bit just involves removing a bunch of form fields. Historically, we haven't considered the list of FormFields in a FiledList to be part of our API. To be on the safe side, we can replace the fields with some dummy hidden fields. That way if you are targeting those field to insert other fields in the form schema, your code will still work.

For the JS part, we still have our giant JS/React cop-out.

In practical terms, InsertMediaModal, AssetAdmin and Editor are so tightly coupled, I don't think it would be possible for anyone to use them for custom purposes. e.g.: The only way I managed to get the InsertMediaModal working with the initial LinkField implementation, was to abuse the Insert Link in WYSIWYG form schema.

Most of the card should be achievable while preserving the current behaviour of the current React props. The only exception would be the "Update current usage of InsertMediaModal to use the new approach".

What is a FormSchema component?

This would probably be

• a specific instance of <FormBuilderLoader /> or
• a method that receives a bunch of parameters and generates a matching <FormBuilderLoader />

About hiding forms

If the form is in a modal, this sounds sub-optimal to me. I'd much rather that modals are only rendered when they're needed.

The <FormBuilderLoader /> is not "directly" in a Modal.

Status quo

That's the current React Dom

• InsertMediaModal
  • FormModalBuilder
  • AssetAdmin
    • Editor
    • FormBuilderLoader (That might render a form to insert an image in the WYSYWIG)

When you click the "Details" button:

• Any data the user has put in the current editor form gets stashed in the Redux store.
• Redux store is updated with a different action for the editor.

The Redux store change triggers changes in the DOM:

• InsertMediaModal
  • FormModalBuilder
  • AssetAdmin
    • Editor (Gets new props that generate the schemaUrl)
    • FormBuilderLoader (Renders the form to edit the file)

When your done editing the file:

• A redux action is dispatched that restore the editor to it's initial state.
  • The Editor will refetch the form schema you were originally editing.
  • This would normally wipe out any outstanding changes you had.
  • That's why we had to stash those value in the first place.
• Redux store calls setSchemaStateOverrides method with the stashed value to force them to be loaded in the FormBuilderLoader.
  • The setSchemaStateOverrides is our own creation that basically allows us to abuse Redux form. It's flimsy and took me several hours to figure out 3 years ago. It's probably likely to break in a future redux form upgrade.

What I want to do

That's the React Dom in my proposed solution

• InsertMediaModal
  • FormModalBuilder
  • AssetAdmin
    • Editor
    • Initial FormBuilderLoader (This is provided by whatever is calling InsertMediaModal. e.g. FileLinkModal)
    • Edit file FormBuilderLoader (not rendered initially)

You decide you want to edit the file:

• Editor's state is updated with useState hooks. That doesn't affect any other component. So there's no value in putting this state in the redux store.

Your React Dom now looks something like this:

• InsertMediaModal
  • FormModalBuilder
  • AssetAdmin
    • Editor
    • Initial FormBuilderLoader (hidden with CSS, all its state is still loaded in the redux form)
    • Edit file FormBuilderLoader (gets rendered now)

When the user is done editing the file, the state of editor is restored. However, because the Initial FormBuilderLoader was never unloaded, it still remembers all its values and no form schema requests are repeated.

In my mind this is more like having a parent form that gets "frozen" when a light box gets displayed on top of it.

Customising the EditorHeader and PreviewImageField

As it stands, you don't have much scope in customising them any way. You could change the header text, I guess, by swapping out the form field.

The PreviewImageField might give you the ability to render a different preview, by swapping out the field with something.

I noticed there's a FileSpecs LitteralField in there as well.

All those fields are visual only. Any functionality associated with them is entirely control in React any way.

If we care about making those customisable, the best thing we could do is register them in the JS Injector and let people override them with their own React component.

Deprecating PreviewImageField

This doesn't have any use outside of the Editor

All this does is provided a bunch of props that ReduxForm passes on to its React component for rendering. It doesn't have bootstrapping logic, so it would render nothing if you tried to use it in a plain entwine form.

Customising the AssetForm factories

You can customise those ... and some people have. FocusPoint being tho most widely used one.

The primary customisation is to use some hooks to add/remove fields to various forms. This is usually done with the insertAfter or insertBefore methods. That's why I think maybe we want to keep those fields around as dummy hidden field.

You can add actions, but the actions themselves don't do anything on the PHP form. The editor high jacks them and then creates its own request to custom endpoints.

Refactoring component as function

Some of those components will probably end up being 60-80% rewritten. In that context, it seems wise to rewrite them with a more modern approach in the process.

In some context, it might also be easier as well. A lot of the lifecycle methods on those class components are doing very simple things in very complex ways. Once you start looking at what they actually do, you often realise that this is just an artefact of someone not really knowing what they were doing when they wrote the component.

Breaking this into issues

Though perhaps "Document how to create custom InsertMediaModal." should include documenting why you might want to do that.

That's the why: https://github.com/silverstripe/silverstripe-asset-admin/issues/1021

Update LinkField FileLink to use InsertMediaModal. Why? What benefit does that provide over the current link field modal? Surely a consistent link editing experience is better than having a completely different modal for each link type?

My initial implementation of LinkField was using the InsertMediaModal. But we switch away from that because we couldn't get it working with the new FormSchema driven approach

The Insert Image and Insert Link feature in the WYSIWYG use InsertMediaModal. So content authors are already familiar with that.

The "Modal inside a Modal" is the thing that is inconsistent.

Update current usage of InsertMediaModal to use the new approach That doesn't seem optional to me? Isn't the whole point that we clean up the mess that's there?

AssetAdmin and Editor still have a lot custom logic to handle these use cases:

• Select a file in an Upload field
• Add a file link to a WYSIWYG
• Add an Image link to a WYSIWYG
• Edit a file
• Edit a folder

You could retain that custom logic and keep AssetAdmin and Editor tied in together. This would arguably be less of a breaking changes.

Alternatively, you could pass an initial FormSchemaBuilder to Editor and remove any responsibility it has to handle the response. In my mind, that's a better approach but it does mean that if there's any mad scientist out there that has high jack those actions for their own purpose (e.g. like my initial FileLink implementation), we would probably break things for them.

[GuySartorelli]

Initial FormBuilderLoader (hidden with CSS, all its state is still loaded in the redux form)

I don't think it needs to be in the actual DOM in the browser - we can still have the component present in the react DOM, retaining all its state etc, and just tell this component "hey you're not needed right now so don't render anything into the DOM". Best of both worlds.

About customising the editor

I think it'd be worth making sure the focuspoint module still works (or is able to be easily adapted to work) with whatever changes we make (see the asset form factory extension - but beyond that I take your point that it's currently prohibitively difficult to customise to the point of making backwards compatibility a nearly-moot point in many cases.

Refactoring component as function

I guess I'm broadly okay with this... so long as it's a "do it if it makes sense in the moment" and not a "you must refactor it regardless of how it ties in with the main refactoring you're doing", and so long as that is done (as much as possible) in separate commits to other refactoring so it's clear what is relevant to just making the thing functional vs the other refactoring that's happening.

Everything else

My initial implementation of LinkField was using the InsertMediaModal. But we switch away from that because we couldn't get it working with the new FormSchema driven approach

I don't know that that was entirely the only reason? It seems to me just a better UX that the modal is the same or at least very similar between different link types. "I'm editing a link, and this is what editing a link looks like". They're not inserting an image anywhere, so we shouldn't use the inserting an image modal - except when they do click "insert image" in the upload field, which is specific small part of editing the link record as a whole.

The linkfield thing does start to edge towards being off topic - let's abstract that a little and say there's some use cases where developers might want to be able to select an image without using the UploadField.... for... some reason? I don't really see the use case but maybe it's there.

With exception to the PR which still has outstanding questions on it about the use case of having a stand-alone modal component, this all sounds sensible to me now that you've given more context.

[maxime-rainville]

We had a meeting about this. Conclusion was to validate whether there's an immediate use case for this.

If there is, the approach highlighted here is generally fine.