Allow user to edit file information directly in the modal

[maxime-rainville]

This PR adds the ability to edit the detail of a file within the file insertion modal.

Other improvement

• Move getFormSchema out of AssetAdmin
• Replace convoluted logic to get breadcrumb data into redux store
• Tweak the header spacing

Parent issue

• Having to "Edit Original File" is unintuitive compared to SS3 inline editing #813

[maxime-rainville]

Things left to do to get this PR over the line:

• Add unit test for getFormSchema
• Add unit tests to Editor to cover
  • handleClose
  • editorHeader
  • createFn
• Add unit tests to EditorHeader
• Update Modal store to cover new state
• Write some behat test
• UX feedback
  • Make placement an header rather than a tab
  • Add a warning about editing files globally in the modal

[maxime-rainville]

GGRGRGRGR!!!!

This seems to have broken the UploadField!!!

[maxime-rainville]

Fixed the UploadField
Added some Behat test to cover it
Rebased with 1 branch

[emteknetnz]

Epic work.

Main thing for me was rename the new Breadcrumb container to something slightly different so that we don't have a confusing situation with two Breadcrumbs

[emteknetnz]

As discussed on slack:

• including frameworktest as dev-dependency in composer WILL NOT require frameworktest unless it's the in the project root.
• means it won't do anything on your local if you're doing development from the vendor folder (as most of us are)
• in the case of asset-admin being the root project, which it will in travis, it is functionally the same, though it's clearer being in travis.yml because it shows the intent better

Seems like the bigger question though is why is frameworktest a requirement for behat tests in the first place? I've always seen frameworktest as a quick and dirty way to create lots of records for manual testing. We're including dev-master here because we don't bother versioning frameworktest

Rather then rely on frameworktest for behat fixture creation, seems like it would be much cleaner to create them the old fashioned way i.e. yml

[maxime-rainville]

There's a precedent for using frameworktests ... it gets installed by the CWP kitchen sink.

I can't just rely on creating fixtures with YML because I need an DataObject with an has_many relationship to Image or File. Off the top of my head, I don't think we have a recipe that has that particular set up out of the box

[emteknetnz]

You can probably guess what I'm going to say next :-)

Just create a TestOnly DataObject in asset-admin and use that. Much cleaner and safer than using frameworktest that has no versioning and could change at any point.

Also means you can run behat tests locally without having frameworktest installed (and which isn't specified any where but in .travis.yml that it's a requirement)

[emteknetnz]

Fairly minor UX issue, happy for this to be created as a new issue resolved sometime later on post-merge.

When I edit a page
And I use TinyMCE
And I click the insert an image button
And I upload a new image
And I click the "Details" button in the top right corner
And I change the "Title" of the image
And I click the "Save" button
Then I expect to see the green button to have the text "Insert image"
But actually it has the text "Update image"

[emteknetnz]

@maxime-rainville
One issue we should resolve before merging - the datetime fields on the image are now editable. They should be read-only

On the main branch, they are read-only in the details tab

[clarkepaul]

Then I expect to see the green button to have the text "Insert image". But actually it has the text "Update image"

@emteknetnz I can't see the actual experience but from what I recall... as the image is already inserted, you are just updating it. If you were to remove it and re-insert it you get the "insert" button label again.

[emteknetnz]

I can't see the actual experience but from what I recall... as the image is already inserted, you are just updating it. If you were to remove it and re-insert it you get the "insert" button label again.

That's correct for already inserted images. However you can still get this 'Update image' (or 'Update file' for pdfs) for "uninserted" Images/Files. It still functions correctly as it will insert the file, it's just it shows the wrong text

[maxime-rainville]

I've got a fix for the two fields that should be readonly.

@clarkepaul Would you have a problem if updated the modal so that the button always says "Insert" even if you are updating an image that has previously been inserted? The logic for this part is kind of clunky (which is why it's broken in the first place). I could probably make it work at the cost of adding an extra layer of clunkiness.

If we're OK with the button always saying "Insert", I can actually remove some clunky code.

[maxime-rainville]

Just had a Zoom catch up with Paul to discuss how to handle the Insert/Update button issue. The 3 options discussed were:

1. Have the button to always display "Insert" no matter the context.
2. Me spending a couple hours to fix the bug with the button by adding some extra hackish logic on top of the existing hackish logic.
3. Live with the bug.

Paul says he's happy to live with the bug since it's not on the primary path most users will take and it's kind of minor anyway.

[clarkepaul]

@maxime-rainville has just walked me through the UX button label bug. I'm okay with leaving the bug as-is for now if we can raise it as a separate issue that would be good.
My reasoning is that for the two most common user paths are catered to:

• inserting an image, the language "insert" makes the most sense
• editing a file when it has already been inserted, the language "update" makes the most sense

From my experience, cms users are more concerned about the presentation of a file initially, editing the file details is a secondary thought (although the main benefits of this issue). Returning without making any edits at all would be subset of people. For the remaining people, I think they would complete the intended action of inserting the file with the button label being "Update" without too many concerns.

Obviously I'd prefer it fixed but I don't see this as a blocker to getting the issue across the line.

[sachajudd]

Nice work everyone!! 🎉

[purplespider]

This is nice! But may I suggest changing the "Details" button to say "Edit" instead? To make it more obvious how to edit the fields?

I'd expect a "Details" button to provide me with more, erm, details, but this button makes the existing fields editable.

[clarkepaul]

Much discussion and back and forth on this one.

• The fields in the current placement view are already editable, you might think you need to click Edit to adjust the current fields in view already.
• With a label Edit original we had comments from users like "What's the original file? why would I want to edit a different file?" The concept of original made it feel like its a copy rather than the same file.

The button takes you to the file Details tab, so having a button with the same name made some sense here. Edit details was also considered but it tested fine without the word edit. This was the terminology which was already being used navigating between the Placement tab and Details tab.

This one doesn't the best experience you could ever want but hopefully it's an improvement from what was there.

[purplespider]

Ah, thanks Paul, after reading your comments, I've realised this button appears in two different screens:

---

1. Above the Placement tab, when dealing with a WYSIWYG image:

In this context, everything you said makes sense and changing it to just say "Edit" would be unclear.

---

2. Above a read-only Details tab, when dealing with an image in an UploadField:

It is in this context where the Details button is currently confusing. As you are already viewing the Details tab.

A primary use case of this button is when using the FocusPoint module.

---

I presume there is a reason why the fields just aren't immediately editable in the latter view.

So, perhaps the best solution is to change the button to say Edit Details so that it's action is clear in both contexts?