feejin commented 6 years ago

When viewing image info the details (Title, Filename etc) appear to be form fields but are not editable, they can only be edited by clicking Edit Original File which opens in a new tab.

It's more noticeable with the FocusPoint module when you want to just make a quick change as soon as you upload the image. Makes for quite a jarring user experience and I have to explain this to non-technical clients.

tl;dr make asset details editable without having to go via Edit Original File.

User story

As a CMS user, I want to be able to stay in the current modal when editing a file, so that I can make quick updates to the file without loosing context of the area related to the file.

Acceptance Criteria

[x] 'Editing' a file keeps the user in the same modal
[x] There is a button that initiates the file in a new editable state.
[x] When editing the file, you can save/publish.
[x] When editing the file, the other actions are also available (archiving, unpublishing, replacing the file).
[ ] A prompt will warn the user that edits will be applied globally.
[x] There is a clear flow of inserting the image after editing.
[x] Editing a file in the modal might change its draft/live state. When you're done editing these changes should be reflected in the rest of the modal.
[ ] This improvement is for the WYSIWYG modal only, and the uploadfield modal should be handled in a separate issue.

Stretch Goal

[x] Implement this feature in the Upload Field Modal or raise a separate issue for it.

Suggested designs

Pull Requests

[x] https://github.com/silverstripe/silverstripe-admin/pull/968
[x] https://github.com/silverstripe/webpack-config/pull/39
[x] https://github.com/silverstripe/silverstripe-asset-admin/pull/1038
[x] https://github.com/jonom/silverstripe-focuspoint/pull/78 (tweaks focus point module to work with the new image placement form)
[x] https://github.com/silverstripe/silverstripe-framework/pull/9425
[x] https://github.com/silverstripe/cwp-recipe-kitchen-sink/pull/63

maxime-rainville commented 6 years ago

That's kind of related to https://github.com/silverstripe/silverstripe-asset-admin/issues/756 and https://github.com/silverstripe/silverstripe-asset-admin/issues/785.

@chillu @clarkepaul Might be worthwhile having an epic for the insert media modal of the WYSIWY/UploadField.

clarkepaul commented 6 years ago

The original concept was to have the details editable, but with a confirmation to ensure people know they are edit globally (eg information possibly used in other placements).

feejin commented 6 years ago

@clarkepaul That would make sense, the global edit warning would be good as it's caught people (me) out in the past in SS3.

undefinedplayer commented 6 years ago

Hi, this issue is significantly affecting our product. Wondering if there is any plan to solve this issue. Or is there any way to extend SilverStripe CMS to achieve this?

lekoala commented 5 years ago

Hey just to add a few ideas from my original issue, here is a few things i'd like to be considered:

there should be a way to edit easily a file details right from the upload field. I don't know for others, but for me, it's very rare to have files used in multiple places, most of the time, a file is linked to one page or one dataobject, which allow me to store/clean files more easily (otherwise, the asset folder tend to become one giant mess). Also, based on ownership, you would know if a global warning is required or not.
when adding new fields (like, a "credits" field for image), it's more of a global field, it should not require warning and should be easily editable right from the upload field.
if the extra fields or changes are to be specific to a relation, wouldn't a many_many relation with extraFields make more sense? (and again, easily editable from the upload field, since the file manager wouldn't know about each specific relations)
previewing files should also be a lot easier (how many times I had to tell a customer how convoluted it is to see the content of their word or pdf document that is linked to a page? yes, click on the "little eye", then click on the download icon ... ah yes, and it doesn't work for draft files! it's like 2 clicks for something that should be a simple click on an icon)

:-) well anyway I think there is room for improvement there!

clarkepaul commented 5 years ago

Thanks @lekoala for the detailed description, this will require some UI changes and it is on our radar to make the improvements. This one is best going through the UX team to review to look at the different options. Totally agree that editing should be possible when uploading a file directly. When it comes to files used in multiple locations we will advise on a solution.

newleeland commented 5 years ago

Potential designs for review

Click the link Editing files: https://projects.invisionapp.com/dsm/silver-stripe/silver-stripe/folder/components/5c68f96ffb26bb0018525d29

bscarola commented 5 years ago

The feedback link above isn't working for me. Is there any timeline for folding this into a release, or perhaps a temporary work-around? I've finally begun upgrading projects to SS4 and I agree that this is a major workflow issue for a lot of clients.

clarkepaul commented 5 years ago

I've updated the link @bscarola. Looks like its basically turning the readonly fields into standard editable fields. When editing an existing file, I've flagged that there should probably be some sort of messaging to let a user know they might be editing a file which is used in several locations—for uploading that message wouldn't be required.

lekoala commented 5 years ago

hey @clarkepaul is it me or this only covers tinymce/filemanager usage? What about regular upload fields directly visible through getCMSFields?

clarkepaul commented 5 years ago

This should cover both ideally, I'd leave separating it into two issues to whoever picks it up. Thanks for mentioning it @lekoala

neilcreagh commented 5 years ago

Just a quick note to say that I think this should be very high priority. Editing an image title (or focus point) at the moment is a very awkward experience - especially for clients coming from SilverStripe 3 who could do this with one click from the uploadField before (and understood this to be an upgrade to their previous version).

maxime-rainville commented 5 years ago

Just had a chat with @newleeland to talk over his design. The design introduces a somewhat different state in the insert modal to edit the detail of the image. This will probably require a bit more work than if we had just made the current read-only form editable, but it should provide a much nicer experience for the user.

This change will reuse functionality and UI elements uses elsewhere. Changes to the back end logic should be trivial ... juts minor tweaks to FileFormFactory. Most of the work will probably be in React.

Some things to keep in mind when implementing/testing this:

When editing the file in the insert modal, you can save/publish. The other actions (archiving, unpublishing, replacing the file) will not be available.
Users who might not have access to edit file, might still insert them. We should not show the button to trigger advanced editing.
Editing a file in the modal, might change its draft/live state. When you're done editing and these change should be reflected in the rest of the insert modal.

clarkepaul commented 5 years ago

I'd like to see how much more effort this would be as I think it would be really useful functionality.

The other actions (archiving, unpublishing, replacing the file) will not be available.

maxime-rainville commented 5 years ago

I don't think it would be technically difficult. It might add more edge case for us to consider.

The thing I'm not sure is if the user will be in a good head space to make those decision. e.g.: You decide to edit the "about us" page, you decide to replace a file without realising it's use on a bunch of other pages.

clarkepaul commented 5 years ago

That's the same issue when in the files area, at least they have the "used on" tab. The way I see it is that as soon as you go away from the "Placement" view you've changed the mindset of what you are doing.

neilcreagh commented 5 years ago

I see this as urgent, so I'd push for the fastest/simplest solution to be rolled into the next minor update - if that's simply changing the the current read-only form to be editable I think that should happen. Even better if there's a warning message about affecting all versions of the image.

newleeland commented 5 years ago

New ~Potential~ Mock designs needing consideration

https://invis.io/WCMZMHHXRJV#/381953634_Files_Tiny_Insert_Files_new_File_start https://invis.io/WCMZMHHXRJV#/381953633_Files_Tiny_Insert_Files_existing_File_start

Comments on any edge cases are welcomed

brynwhyman commented 5 years ago

Looking at the new designs... is the intention to remove all of the read-only tabs except from the 'Placement' tab, until the user edits the image?

I would have thought this information would still make sense for users without access to edit files to see?

clarkepaul commented 5 years ago

We removed the tabs from the place view so there is no confusion with all of the actions... we can't have Insert mixed with Save/Publish but you do have a good point. We can't keep the preview tab when going to the edit view so there would be some inconsistency with what tabs are showing (unless we put placement into read-only).

How about if the action at the top was "File details" (or "Details" for short) implying it is for both viewing and editing?

We could leave the read-only file details in the placement view but it is only a click away, we thought the separation of placement and file details was the best thing to do here. In saying that there might be a better UI for navigating between views than we have here.

lekoala commented 5 years ago

@clarkepaul I see this issue is still only about usage of files inside TinyMCE. Was there another issue created to track usage in file uploads? It might be just me (but I don't think so!) but for me, it's also a matter to easily edit files attached through UploadFields. In SS3, it was really easy to do so, and more specifically adding new fields for an image ("credits" fields for instance). For instance, how would a "image cropper" module be implemented with the current designs? I wish there would be a way to have files simply scoped within the page or dataobject they belong with. Shared assets is really not my common use case : most of the time, my files only make sense in the context of a specific page or dataobject.

clarkepaul commented 5 years ago

@lekoala Yup this caters to both TinyMCE and Uploadfield for editing, but placement details only apply to TinyMCE placement, we are considering how to attach ALT text field to UploadField items but not included in this work. Here are some of the issues for the file usage tab information https://github.com/silverstripe/silverstripe-asset-admin/issues/943 https://github.com/silverstripe/silverstripe-asset-admin/pull/998 https://github.com/dnadesign/silverstripe-elemental/pull/734

These proposed designs will allow images to be edited from the context of the page. This issue doesn't go into how to add additional fields but I don't see this as being a problem UX wise.

tractorcow commented 5 years ago

I tried restoring the edit feature when edit mode is "TYPE_SELECT", which restored the editability, but causes javascript errors when trying to save.

tractorcow commented 5 years ago

It's because ACTION_TYPES.UPLOADFIELD_ADD_FILE is triggered when saving. UploadField treats any action as "insert", not only the actual "insert" action.

tractorcow commented 5 years ago

This is what I would like to get working.

This is the error I get when trying to save.

@unclecheese do you have any insights? Is there a "quick fix" to allow edit mode to work (when manually turned on)?

newleeland commented 5 years ago

Looking at the new designs... is the intention to remove all of the read-only tabs except from the 'Placement' tab, until the user edits the image?

I would have thought this information would still make sense for users without access to edit files to see?

We've rename the button to "File details" to accommodate this secondary flow of viewing and editing file details.

tractorcow commented 5 years ago

I've tracked the issue to InsertMediaModal.js (used by uploadfield).

  /**
   * Handles the insert form submission, does not continue the regular form submission within the
   * asset admin section.
   *
   * @param {object} data
   * @param {string} action
   * @param {function} submitFn
   * @param {object} file
   */
  handleSubmit(data, action, submitFn, file) {
    if (action === 'action_createfolder') {
      return submitFn();
    }
    return this.props.onInsert(data, file);
  }

Basically, any action other than "createFolder" is being treated as an insert.

tractorcow commented 5 years ago

git diff to enable editable files during insert:

diff --git a/code/Forms/FileFormFactory.php b/code/Forms/FileFormFactory.php
index 2e4f884..d113b5c 100644
--- a/code/Forms/FileFormFactory.php
+++ b/code/Forms/FileFormFactory.php
@@ -66,7 +66,7 @@ class FileFormFactory extends AssetFormFactory
                 $tabs->unshift($this->getFormFieldLinkOptionsTab($record, $context));
                 break;
             case static::TYPE_SELECT:
-                $tabs->setReadonly(true);
+                //$tabs->setReadonly(true);
                 break;
         }

@@ -281,6 +281,8 @@ class FileFormFactory extends AssetFormFactory
         $actionItems = [];
         if ($type === static::TYPE_INSERT_MEDIA || $type === static::TYPE_SELECT) {
             $actionItems = array_filter([
+                $this->getSaveAction($record),
+                $this->getPublishAction($record),
                 $this->getInsertAction($record),
             ]);
         } elseif ($type === static::TYPE_INSERT_LINK) {

tractorcow commented 5 years ago

This fixes it. :)

  handleSubmit(data, action, submitFn, file) {
    if (action === 'action_insert') {
      return this.props.onInsert(data, file);
    }

    // Standard form actions (e.g. publish)
    return submitFn();
  }

tractorcow commented 5 years ago

Bugfix to the specific JS error at https://github.com/silverstripe/silverstripe-asset-admin/pull/1005

Note: This does not address the major issue, which is the negative user experience out of the box. :)

tractorcow commented 5 years ago

By the way, I got the above working in my client project, and the client is very happy with the result.

In the upload field, you just need to click the "eye" icon beside any uploaded file, and you can immediately edit the file (as per the above screenshot).

You get all 3 menu actions (save, publish, insert) beside each file.

clarkepaul commented 5 years ago

@tractorcow we choose not to do that for the Editor insert as the positioning is not part of the versioning for the file, but probably all good for the Upload Field. Do you know if there's any information for a file which is likely is added to an Upload Field which wouldn't be versioned? (e.g. do developers add positioning, additional fields like ALT text).

I'm thinking for a file to have an ALT text field included in the versioning so it can be used in the Upload Field instance. Do you have some other edge cases for Upload Fields which you might have seen before?

tractorcow commented 5 years ago

@tractorcow we choose not to do that for the Editor insert as the positioning is not part of the versioning for the file, but probably all good for the Upload Field though. Do you know if there any information for a file which is likely is added to an Upload Field which wouldn't be versioned? (e.g. do developers add positioning, additional fields like ALT text).

I would probably version everything regardless.

My use case is only for uploadfield assignment, I agree with you regarding the html editor media dialog. :) In the case of uploadfield though, the data for that record can only be stored on the file (unless we are somehow adding metadata to the uploadfield assignment; Possible future enhancement).

maxime-rainville commented 4 years ago

How I'm thinking of approaching this:

Update FileFormFactory so it doesn't return all the fields for the Insert Media and Insert Link form and returns a "Edit original" form action instead
Update client side library to high jack the "Edit original" form action to
- Create an Edit details button component
- Add a state to track which form schema we are showing
- Add a state to track whatever the "edit details" buttons should show in Editor
- Add an action to Editor to trigger the edit state form schema (I'm thinking we might want to have a separate API to register action handler otherwise it's impossible for third party to specify their own actions)
- Update existing actions so they can restore the old form.
- Add an action to cancel edition
- When we dig deeper switch form schema being displayed, we need to keep track of the current data we're showing.

maxime-rainville commented 4 years ago

Just a quick update on where I'm at:

I'm displaying the simplified form when inserting an image or a link
You can click on a Detail button to edit the file
You can click on a back button to return the insert media form

Next steps:

Returning user to the insert media form when saving the image ... and other appropriate behaviour based on what action the user as selected.
Storing the Insert Media form state before showing the Edit form ... and restoring it when you get back.

neilcreagh commented 4 years ago

I think the UploadField issue is a much simpler approach, leaving everything behaving exactly as-is but allowing editing from the Uploadfield without the 'Edit Original File' button (And perhaps with a sentence explaining that this will edit the file anywhere it is currently used).

Should the Uploadfield have a separate issue? The original post here says "This improvement is for the WYSIWYG modal only, and the uploadfield modal should be handled in a separate issue." but a couple of issues that have been opened about it have been closed as duplicates of this?

maxime-rainville commented 4 years ago

@neilcreagh I'm half way through fixing this. From looking at the WYSIWYG modal, I don't think it's going to be terribly difficult to get the Upload field working. I'll add stretch goal to "Implement this feature in the Upload Field Modal or raise a seperate issue for it".

Once I'm done, I'll be in a better position to know how difficult it's going to be.

maxime-rainville commented 4 years ago

@silverstripeux I got this in something resembling a usable state.

https://youtu.be/dGOMiyzzekI

Here's a few notes:

The mock-ups have a "editing file x" header when editing the file in-modal. Adding this message only in the modal will make the logic somewhat inelegant. Would it be OK if I add it to the main asset area as well? Or if I drop it altogether?
I've altered the way we display the title header field. This will also affect the asset-admin area.
Saving/Publishing the original file doesn't currently automatically send you back to Placement form. Is that the behaviour we want? What about all the other actions?

Warning when you're about to switch form

Right now, when you do an action in Asset-Admin or in the asset modal that will cause you to loose some changes, you don't get any warning. That's what in modal editing is doing right now. Maybe it's a bigger card, that need to be address somewhere else. Maybe we can address part of the issue here first.

If I start editing the placement information and then I edit the file details, what should happen?

Block the user from editing the details until they save the placement info?
Show some warning to the user?
Stash the form info and try reloading it once you're back from editing the file details?

We'll have the same problem with the edit detail form.

Using/Abusing our new form editing powers

The way I've approach this, we could in theory get a form that opens another form, that opens another form, etc.

For example, we could use this to display "Add to campaign" form in the asset-admin panel instead of using a separate modal. Or we could display delete/unpublish confirmation message for the file you are currently editing there.

clarkepaul commented 4 years ago

Thanks @maxime-rainville I'll have a look and get back to you this week.

maxime-rainville commented 4 years ago

@sachajudd Already gave me some feedback and I made a fair bit of progress since I wrote that message. Maybe hold off looking into it until tomorrow. I'll try to get my updates into a presentable state and you can review the greatest and latest.

lekoala commented 4 years ago

@maxime-rainville any update on dealing with UploadFields and allowing editing files from there easily in a customisable fashion?

martinviolet88 commented 4 years ago

@maxime-rainville any updates?

brynwhyman commented 4 years ago

Hey @lekoala , @martinviolet88 you could check the related pull requests (in the original description) for some more detail on the progress of the issue. Currently the focus is still being spent on the WYSIWYG modal, before the UploadField modal is looked at.

There's a few related PRs, but if you would like to check these out and provide any feedback/ test notes, that would be much appreciated!

emteknetnz commented 4 years ago

Ran it all of the combined PR's through travis cwp-recipe-kitchen-sink - there was a single regression

https://travis-ci.org/github/creative-commoners/cwp-recipe-kitchen-sink/builds/676011738

Note: ignore the failure in 'recipe_content_blocks' that's an existing failure

Note: this is a kinda special travis build, you can't restart the build to get the latest changes, you need to run composer update on your local and recommit the composer.lock file

maxime-rainville commented 4 years ago

Everything is done. There's a related PR against the focuspoint module. I've let the module maintainer that it's coming.

maxime-rainville commented 4 years ago

FYI The related focus point PR has been merged. https://github.com/jonom/silverstripe-focuspoint/pull/78

silverstripe / silverstripe-asset-admin

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

User story

Acceptance Criteria

Stretch Goal

Suggested designs

Pull Requests

Potential designs for review

New ~Potential~ Mock designs needing consideration

Warning when you're about to switch form

Using/Abusing our new form editing powers