Closed laryn closed 4 years ago
Rough image mockups for discussion (no code yet).
I would sure like to see that. I've always thought you should be able to select from already uploaded images anywhere you insert images. I have always had to install additional modules in Drupal for that. (Insert module I believe). I've never liked having to re-upload redundant images for inserting when they are already sitting there in the library.
This makes sense to me. It's not clear to me why it makes any more sense to offer this option in the WYSIWYG editor than it does to add it to the file field. I've already looked for it there, only to remember it's only available in the editor.
I also like the idea very much!
@laryn - For reference I'd suggest to also have a look at #1307 (part of the meta issue #1448) which sounds quite similar. I'm not sure if it makes sense to continue the work there, or if it's better to restart with a more simple approach which makes no claim to be complete.
I like this idea a lot as well 👍 ...tentatively setting the milestone for 1.12.0 😄
I like this idea very much and would be glad to see it implemented as soon as possible. This issue in one of my major inconveniences when using Backdrop.
I agree, this looks fantastic, and if its easier to start from this angle (rather than https://github.com/backdrop/backdrop-issues/issues/1307) I'm all in favor of it. Progress is progress!
@laryn will you have time to work on this before Jan 1st? (Or, will anyone else?) If so I'd love to prioritize this for the next release. If not we can bump to 1.13.
The code for the toggle between different file selections is very specific to the image dialog right now (I believe the code for it is also in filter.module). I'd like to see that code refactored into something generic and made a part of File module (essentially FileField Sources) if we're going to use it in multiple places. But that is a lot of work. Maybe they don't need to be the same thing? I've definitely been bitten by the "hey we have almost the same thing in two places" bug only to have the refactoring create something much messier than just having two different implementations.
@quicksketch I don't see how these can be different things. Both the editor toolbar button and the "Choose file" button should allow either uploading new, or selecting existing files. It's just that the one is to insert the image to the file field, while the other is to also generate the required HTML to insert it in the body/text field.
Having said that, I don't know the complexity behind actualising this. I would need to see a PR to understand if the code we end up with is spaghetti or not.
@klonos I guess @quicksketch talk specificly about the JS code which handle the pseudo-tabs widget for the toggle between upload or library. This JS lives in the filter.js (from filter module).
anyway +1 for the main idea of having a library modal on any image field. This is what a real CMS should do ! (you know, some CMS does not even have a wysiwyg in core so ... :p )
We should allow image styles to be selected from the dialog as well (#3589), which will basically be https://www.drupal.org/project/iss
@laryn I have started to work on this, adapting code from the Image Library implementation in the filter module, with which I have some experience. I am working on adapting the necessary javascript at the moment. I aim to have a proof of concept PR by end of October 2019. I am away on holiday next week so cannot do more until after 7th October.
@Graham-72 Sounds amazing -- thanks again!
We also need to update the the image library to allow inserting other media.
Yeah, and perhaps start calling it Media Library instead 😉 ...or File Browser(?).
@laryn Initial PR now published at https://github.com/backdrop/backdrop/pull/2953.
Cool. I'm happy to see some work being done on this issue. :-)
Great, thanks @Graham-72 -- I'll pull out some screenshots and a few comments here. Anyone else feel free to chime in, too.
1) Field display when adding a new piece of content with an image field:
Comments:
2) Field display after clicking "Select from library":
Comments:
3) Confirming selection of an image from the library:
Comments:
4) After confirming an image:
Comments:
@Graham-72 – would it help to have graphical mockups to work from, or do you have a pretty good sense of direction you'd like to go next for these screens?
@laryn thanks very much for your comments. I will be making some modifications soon. As you suggest I think we can leave final styling until we have settled some other issues.
I think we ought to provide for setting of the alt and title fields as part of the process, something I don't think we can do at present?
The Alt and Title from the image field, or the Alt and Title fields from the File entity? We do the former currently, but not the latter.
Yes, an important point. If we reuse an image by selecting it from the image library, should we also be reusing its Alt and Title field settings, or should these be set for each use?
It would be great if the alt/title values set when uploading an image were set as global defaults for that image and loaded by default on additional uses (but could be overridden at that point in a per instance basis)
Also must take note of issue #4007
I have made some changes to the PR and have more in the pipeline. I would like the screen 'Confirm image' to show after the file upload in the same way that it does after selection by clicking in the image library, and then to give the opportunity to enter alternate and/or title text.
Today in the weekly meeting @quicksketch asked if the image gallery was opening in a modal, and it looks like it isn't right now. Is that part of the long-term plan?
I started by thinking it would be in a modal but now I'm not sure whether that is necessary or what the benefit would be, but yes, to be considered.
I also am not sure a modal would be better UX than what we have now. A modal makes more sense in the editor.
I would like to see the thumbnail view behind an Ajax call so that it's not loaded or rendered on an edit form if it's not needed.
The results of the view will also change for the second image field when an image is added into the first one, and rendering the thumbnail view too early would miss that. If you wanted to add an image once but use it twice, you'd need to save the form and edit again to see the new image in the thumbnail view.
On Thu, Oct 31, 2019, 10:56 PM Graham Oliver notifications@github.com wrote:
I started by thinking it would be in a modal but now I'm not sure whether that is necessary or what the benefit would be, but yes, to be considered.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/backdrop/backdrop-issues/issues/3297?email_source=notifications&email_token=AADBERZX7MM546IMBMW4FHDQRPAHXA5CNFSM4FWSDL6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC2CCSY#issuecomment-548675915, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADBER3KOE72SPMQZU7XYZLQRPAHXANCNFSM4FWSDL6A .
I also feel this would be better in a modal. Better UX, especially if you have a lot of images.
I have made some revisions to the PR but am now unlikely to do anything for a few days because I will be away from base. My next intention is to get the alt and title fields working. Other things later - step by step approach.
Better UX, especially if you have a lot of images.
I will need convincing on this.
Regarding modal dialog or not, at the current state of the PR it's hard to evaluate different interface approaches. Let's however keep the question in mind! In the meantime we could have a look at how Drupal 8 and WordPress manage their image / media libraries.
I have made more revisions to the PR. The sandbox demonstrates that it is working successfully though there are a large number of test failures which I will now investigate.
The alt and title fields are usable though need to be reviewed along with issue #4007 and I have made no attempt to store 'initial' values that are re-usable when re-using an image from the library. Where would these values be stored?
I have included my personal preference of showing additional information about each image in the library because of situations where the same image appears more than once in the library, perhaps having a different size - see issue #3386 for examples. To include this in full I had to import into the sandbox a revised copy of the image library view config settings.
Current draft walkthrough/snapshot:
One thing I notice is how the selected image lacks a "Remove" (or "Cancel") button so there doesn't seem to be a way to unselect it after initial selection.
@laryn thanks for the excellent walkthrough-screenshot.
Better UX, especially if you have a lot of images.
I will need convincing on this.
both aesthetic and practical. If you have a gallery site with 500 images this will really clutter the node/add
form. Furthermore it seems that all the thumbnails are loaded on page load, but hidden until you click the library toggle: with many images you will be loading every single one each time. Even worse if you have more than one image field.
(unless there is a plan for some AJAX paging)
Plus we may need more actions on this now or future. How about searching for an image, or sort by size etc? Or selecting an image style? I feel like selecting and sorting and searching for images is an action that should be best performed outside the node edit flow.
Maybe personal preference, but I think a modal is best.
Yes, it should be paging the images as it does for us in CKeditor but I guess this needs to be in a modal to work. At the moment if you have more than 20 images (value as preset in the view) the 'next>' button will not cause the next page to appear. Worse, it crashes the sequence.
We have issue #3293 about searching within the image library. This is marked as 'good first issue' but my attempts have not succeeded so far. It would be good to provide this.
Update from @Graham-72 for meetings today: At this point "...opening a modal is not just preferable but essential in order to get the paging of the Image Library to work. I have also been looking into adding a search on filename field - issue #3293 but have hit a blockage. Regarding the modal, it seems I am going to have to open it from within the javascript because of the constraint of building on the basic file upload dialog. The ckeditor.js javascript file would seem to do this so there is something I can adapt, but it is non-trivial!"
@Graham-72 has made progress on an alternate PR which opens in a modal. Functionality is not all in place yet and I've posted some preliminary comments on the PR itself.
He has added a filename filter to the core Image Library view, and I've added exposed sort for date and name, for discussion. The revised View is attached here as a text file in case anyone wants to import it. (Exposing sort fields is broken and takes a few extra steps to get to work so importing the view on a local may be quicker than recreating). This works in the new modal popup he's working on, but breaks functionality in the WYSIWYG image modal for some reason.
ImageLibraryView-ExposedFilters.txt
Here is a quick demo walkthrough of the current status:
Latest status here and here:
Things proved more difficult than I expected. I still have to arrange for closure of the modals with necessary updating of fields. Some of these things are very hard to find out how to do! You may not be keen on the revised UI and I am not sure how it might be improved without more knowledge and effort. And we still have to tackle the tests, presumably adding some.
I took a look at the latest PR adjustments, which add an auto-complete field to the mix:
I suggested perhaps using Filefield Sources as guidance on the UI here rather than having everything showing:
Also I posed the question of whether the autocomplete should be discussed as a separate/later addition to focus on the image library piece and see if it's possible to get that into 1.15.
Help/direction on the tests will be desirable when we get to that point. Any other comments from others at this point?
Sorry for coming late to the party. I had a high-level play with the current implementation in the PR sandbox, and my first reactions were the following:
The general idea being to reuse UI patterns, and to minimize the space the add image form widget takes, by moving everything into a dialog. The widget should be reduced to a single "Add image" button, an image thumbnail (if an image is selected), and the help text.
Other feedback:
@klonos how many of these are blockers, and how many can be follow-ups?
@jenlampton I see the following as blockers:
Can be follow-ups:
The bit where we consolidate everything into a single dialog that opens via a single "add image" button is also a biggie I imagine. Consider multi-value fields, and how huge the forms will end up being.
BTW, this seems to be a duplicate: #1307
Note to self: copy any useful comments from that issue, and then close.
Image info/details can be added later (will reduce code/scope).
Agreed, I found this behavior to be confusing when no images are available and/or no image had been selected yet.
The dependency on hover also may not work well on mobile devices.
Autocomplete can be a separate issue.
:+1: I think it makes more sense to have the filtering occur after the dialog has been opened (via exposed filters) than to put an auto-complete on the node form itself.
I read over the PR.
Instead of extending the form in image_field_widget_process()
, I think it would be preferable to modify the file_managed_file_process()
function to add new form API properties. We could make it so that something like this would be acceptable:
$form['image'] = array(
'#type' => 'managed_file',
'#upload_location' => 'public://example',
'#browser_view' => 'image_library',
);
Doing this would allow us to style the file upload element all-together, sort of similar to how FileField Sources works (which also extends the "managed_file" element).
For the UI, I think if we can reduce the visual clutter to something as simple as this:
As @Graham-72 already has found, we have to use a button element to open the dialog because it has to be submitted via POST to send the form data. So to make that UI work we'll actually still use a button like we are currently, but hide it with JavaScript.
I know that's a lot to ask at this point. I'll try a hand at this over the next few days and see if it's feasible.
@quicksketch thanks for your comments. I have just added my latest changes to the PR and things now look somewhat different.
I have not managed to complete the modals and have been going round in circles for many days trying to understand and apply the limited Backdrop documentation on these. I have an ambition to master them and write a tutorial with full examples of how to save, close or cancel them!
modify the file_managed_file_process() function to add new form API properties
This sounds a great idea and should make things easier. Thanks.
@laryn please have a look at the revised UI but you will need to be sure your browser downloads the revised file.js
. I would go for a new sandbox but this would mean having to upload a new set of images.
@Graham-72 I posted a new PR at https://github.com/backdrop/backdrop/pull/3028 that takes your work and continues to move things around:
managed_file
element now has a generic ability to use a file browser based on a view through setting a $file_element['#browser_view']
property./admin/structure/types/manage/post/fields/field_image
:
I had to wrack my brain a bit to figure out how to get dialogs to pass information to a different part of the form. I don't think we have any examples in core of a situation like this. CKEditor's file dialog is all client-side JS. And modules like Layout and Views are constantly updating a partially-saved object in the state
table. This situation is server-side based, but we don't have the object store to update. So the implementation looks sort of like a mix between a CKEditor modal and a Views modal.
To set the selected image in the node form, I used server-side callbacks to pass first a selector for the hidden FID element, and then to pass back the selected FID. The modal dialog:afterclose
event is then used to set the updated FID, and then it clicks the hidden "Upload" button to trigger updating the form. This is the same trick FileField Sources uses to set files, that it just updates the hidden FID field and then submits the upload normally. File module will automatically pick up any manually set FID and uses it as though it were an uploaded file.
This PR is still doing a bit too much. The file preview information is image-specific and wouldn't work for generic file selection. I think the closer we can get this to match the experience of the CKEditor image dialog, the better. So as we don't have that in CKEditor, I don't think we should be adding it here either.
@Graham-72, I'd love your feedback. As there is still work I know needs done I'll iterate on this a few times. I'm using your initial dialog code but then the form submission approaches from FileField Sources. If you have implementation questions, post them as comments on the PR and I'll answer them there.
I really like where this is headed implementation-wise, working towards making this a generic file browser 👍 (as opposed to image-specific).
I could not test the actual browser dialog, because as I mentioned in the PR, I could not get it to open. Nevertheless, I have the following feedback:
The browser should also be available when selecting default images/files for fields (and also things like the site logo and favicon - but all this can be follow-ups):
In the screenshot provided by @quicksketch in the previous comment, the label of the standard upload widget is "Browse...", whereas in the PR sandbox it is "Choose file". IMO, this does not differentiate it enough from the "Select existing file" label of the newly-introduced button of the file browser:
...I understand that "existing" is meant/implied here in the context of the site/CMS, but this seems like a point that could potentially confuse users. I have tried a few options, but do not have any specific alternative ATM, as everything I came up with was too wordy 😞. This can be a UX follow-up though.
I have tried adding a second image field in the same form (allowing unlimited values, if that matters), and have noticed that when clicking the "Select existing file" button of the second field, the "Choose file" button of the first field was momentarily disabled (whereas I would expect the matching one to do so):
...since as I mentioned, the dialog would not open for me, I could not test whether the actual file upload goes to correct field or not.
in the PR sandbox it is "Choose file". IMO, this does not differentiate it enough from the "Select existing file" label of the newly-introduced button of the file browser:
What about something like "Select from library"?
the label of the standard upload widget is "Browse...", whereas in the PR sandbox it is "Choose file".
The "Browse..." vs. "Choose file" wording is browser-specific. Unfortunately file upload elements do not let you specify the label of the button. While you can hide the entire element and make a replacement, the label isn't something we changed in this PR.
the "Choose file" button of the first field was momentarily disabled (whereas I would expect the matching one to do so)
Ah! Great catch! So a work-around File module includes is that it disables upload fields during AJAX requests to prevent files from being uploaded when unrelated buttons are clicked. For example files aren't normally uploaded when you click "Add more" on a different field. However the selector makes an exception for the "Upload" button, which clearly should include the selected file. The "Select Existing File" button is close enough to that selector that it also has the same behavior. You're absolutely right that we should disable the file upload when that button is clicked. This entire problem of selecting but not actually uploading is fairly uncommon at this point though, since file uploads now auto-upload immediately after selecting a file. That was more of an issue in D7 when you had to explicitly click "Upload" after selecting a file.
@olafgrabienski noted in the PR comments:
I've started to test the PR in the sandbox site and could open the modal only once, then it didn't open anymore. When I click the Select existing file button, the cursor jumps to the Title input of the node form.
I think this is a larger issue that happens already with all buttons on the node form. It seems some recent changes to Firefox made it so that the browser jumps to the field that has focus when any button is clicked. You can suppress this behavior by first focusing the button with the tab key, then clicking it. As I think this happens with all buttons, not just the browser button, we may want to track that separately.
Describe your issue or idea
I was adding the filefield_sources error to my (long) rainy day list and then had a different thought. How hard would it be to add an option/link to core image fields that pulls up our shiny new media library view in a modal? That would be much better than the Filefield Sources solution, I think.
So you could upload a new file (as usual), or click a link to browse the library and attach an existing image to the file field you're using.
Weekly update? Latest updates on 4th December 2019. A new PR is under development and test at https://github.com/backdrop/backdrop/pull/3011. At the moment it is only partially complete but it does open dialog in a modal. More updates to this new PR today, 18th December. Still not complete. Advocate: @laryn
PR by @Graham-72: https://github.com/backdrop/backdrop/pull/3011PR by @quicksketch (based on @Graham-72): https://github.com/backdrop/backdrop/pull/3028