Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[UX] Add image browser in the WYSIWYG 'Insert Image' dialog. #3134

Closed
Graham-72 opened this issue May 19, 2018 · 93 comments
Closed

[UX] Add image browser in the WYSIWYG 'Insert Image' dialog. #3134

Graham-72 opened this issue May 19, 2018 · 93 comments

Comments

@Graham-72
Copy link

Graham-72 commented May 19, 2018

I am aiming to provide a simple addition to core's filter module to offer the selection of an image from those already stored in the website. I am basing this on work done by @daggerhart in response to issue #1307, published as PR 1889.

I will provide a fresh PR as a proof of concept, for team review, particularly of usability.

Update: My PR 2198 is now working and does, I believe, provide a useful addition to the Insert Image process, particularly for non-technical users.


PR by @Graham-72: backdrop/backdrop#2198

@Graham-72
Copy link
Author

I have my initial PR (number 2198) now working in the sandbox but only by adding contrib module 'autologout' to the sandbox! I have yet to discover why this makes a difference but discovered the need for it, or something, because I found my PR worked on one of my test sites where this is installed.

Without this, there is some interaction of scripts that halts the process.

So there are now two things to do:

  1. Get this to work without the supporting(?) contrib module.
  2. Test and revise the UI in the light of comments and experience.

@Graham-72
Copy link
Author

Thanks to the comment by @docwilmot that the view will work if it does not use ajax, the sandbox is now working to my satisfaction.

@olafgrabienski
Copy link

olafgrabienski commented May 28, 2018

The PR sandbox works for me but I found the help text in the dialog (Remember to set the image style at the foot of this dialogue before selecting Insert.) confusing:

  • I was asking myself what is 'image style' (and guessed Image size and alignment but that's not clear in my opinion.
  • Scanning the text and reading the word "select", I thought that I had to set the 'image style' before selecting the image, which is not the case. Maybe choose another verb for "select"?
  • If you update the image, the button is called "Update" instead of "Insert".
  • Do we need the help text at all? I see that the list of image thumbnails can become quite long. So what about placing the image configuration options at top of the list?

@Graham-72
Copy link
Author

@olafgrabienski Thank you for your comment. The problem I foresee, particularly for non-technical users, is that when the library is showing several images, the fields for setting image width and alignment are not always visible unless one goes to the foot ot the dialogue, and one may miss these important settings.

I think it would be possible to place these above the images which might perhaps be better.

@Graham-72
Copy link
Author

I don't like to use the colloquial word 'click' so what should one say (rather than 'select') about using the 'Insert' button?

@docwilmot
Copy link
Contributor

@Graham-72 I thought that a better option, instead of embedding the gallery in the image dialog, we could put a button that opens a new dialog with the gallery in it. I actually tried to do this, but had an issue with the galley submit button giving a POST error. Was hoping @quicksketch could help, posted in gitter, waiting a response.

@Graham-72
Copy link
Author

@docwilmot yes that might well be better, particularly if the gallery could give some better user feedback that a particular image has been selected.

Whilst on the subject of possible improvements, I think it would be nice to include an autocomplete for anyone keying in a filename, though image files can have very unhelpful filenames.

@Graham-72
Copy link
Author

In the light of comments received (thank you) I have made some revisions to my PR for a version which embeds the image display in the image dialog.

I am particularly keen to get some UI like this that enables a user to see and re-use images that have already been uploaded to a site, so I will continue work on this.

@olafgrabienski
Copy link

@Graham-72 I've had a look at the sandbox site of backdrop/backdrop#2198 where I couldn't find any option to select an existing image in the CKEditor image dialog.

@Graham-72
Copy link
Author

@olafgrabienski I think there are some possibilities of the image library not appearing and maybe the script needs some refinement, but when in the 'Image File URL' dialog you should see the existing images displayed below the subheading 'Select from image library' as in this screenshot:
screenshot-2018-6-6 edit page test page 2 pr 2198 backdrop backdrop testing site

@olafgrabienski
Copy link

@Graham-72 I've looked for existing images in the sandbox site but they're not there, see screenshot:

screen-backdrop-pr-2198-ckeditor-no-images

@Graham-72
Copy link
Author

@olafgrabienski could it possibly be that your PC has not yet refreshed its copy of the filter.js script?

@olafgrabienski
Copy link

Thanks for the hint @Graham-72 , that was apparently the case. I've tried it in another browser and could see the existing images.

Just one first note for the moment: While I could choose from and insert one of the existing images, I was not able to use the image library pager. When I chose the next page, the link opened in a separate normal page, not within the image dialog.

@Graham-72
Copy link
Author

@olafgrabienski thanks for the feedback. I think the paging problem will only be solved if we can get ajax working for the view.

@Graham-72
Copy link
Author

@olafgrabienski I have made a further change to the PR, following an excellent suggestion by @docwilmot. Please have a look at the sandbox when convenient.

@Graham-72 Graham-72 added this to the 1.11.0 milestone Jun 7, 2018
@laryn
Copy link
Contributor

laryn commented Jun 7, 2018

[cross-posted on the PR accidentally]

@Graham-72 Looking good! A few suggestions for consideration:

  • How do you feel about using a square "scale and crop" style so all image previews are the same aspect ratio and we can get a nicely aligned rows and columns?
  • If the width of .library-view is not hard-coded at 600px and we change from a grid display format, it would allow wider screens to display more images without requiring a scroll.
  • Having a simple exposed filter for filename contains would be nice.

@bjoern-st
Copy link

@Graham-72 Great Work!
Just a question: how about an additional (text)field where you can set the margin around the image?

@docwilmot
Copy link
Contributor

How do you feel about using a square "scale and crop" style so all image previews are the same aspect ratio and we can get a nicely aligned rows and columns?

Unless we can show previews in the dialog, we probably shouldn't do this. A content editor might need to know the image aspect ratio when choosing an image.

If the width of .library-view is not hard-coded at 600px and we change from a grid display format, it would allow wider screens to display more images without requiring a scroll.

I dont get what this means.

@laryn
Copy link
Contributor

laryn commented Jun 8, 2018

@docwilmot Screenshots attached.

Current PR (width hard-coded to 600px):
screen shot 2018-06-08 at 10 54 02 am

Slight change (setting width to auto):
screen shot 2018-06-08 at 10 54 24 am

Greater change (changing view to unformatted list instead of grid and using css to display with flex, etc.):
screen shot 2018-06-08 at 11 01 47 am

@Graham-72
Copy link
Author

@laryn I like the change to use a view that is an 'unformatted list' rather than a grid. But I am not keen on using a 'scale and crop' style because I like to have a true preview of the image aspect ratio.

@laryn
Copy link
Contributor

laryn commented Jun 8, 2018

Chacun à son goût. Let me know if you want me to turn the rough into a more formal submit to your PR or if you plan to just work from what you have.

@Graham-72
Copy link
Author

Thank you for feedback and suggestions, some of which are now included. Others require more work, e.g. to solve the Ajax problem.

I am wondering whether there should be an admin configuration setting so that the display of the library view is optional and an alternative might be an exposed filter for filename contains as suggested by @laryn.

@olafgrabienski
Copy link

@Graham-72 Unfortunately I've got little time these days but here are some ideas on the fly:

  • Maybe change the tab name Image file URL to something that makes more clear that there is an image library?
  • On the image library: What about displaying square images and mentioning the aspect ratio as text?

@Graham-72
Copy link
Author

Thank you for all the changes to this PR. It is looking really good except that it now seems to be inserting the thumbnail of the image rather than the stored image file, so the necessary re-calculation of the URL has perhaps been removed?

@quicksketch
Copy link
Member

Doh! Yes, looks like I may have broken it. I can correct that tomorrow. Can you keep looking and see if the other changes look acceptable?

@Graham-72
Copy link
Author

Graham-72 commented Sep 2, 2018

If I add an image at the foot of the edit window and right align it, it does not seem possible to add anything after it because CKEditor will not allow an insertion point after the image. This seems to be the case whether or not there is a caption. I wonder whether this is a more general CKEditor problem because I have experienced something like this in other circumstances.

And the same problem exists if the image is left aligned.

@olafgrabienski
Copy link

it now seems to be inserting the thumbnail of the image rather than the stored image file

Also, the thumbnail has an absolute URL (http://example.org/files/styles/thumbnail/public/inline-images/dummy.png) and is missing the data-file-id.

@quicksketch
Copy link
Member

quicksketch commented Sep 2, 2018

Thanks folks, I've updated the PR to increase the robustness of the relative-URL generation. I don't think it's entirely safe to attempt to reverse-engineer the original URL out of an image style. Some modules like CDN or S3Connector might make local URLs for image styles but remote ones for the original images. So we need to separately include both the thumbnail URL and the full URL (via a data attribute), then attempt to strip off the domain name for portability.

If I add an image at the foot of the edit window and right align it, it does not seem possible to add anything after it because CKEditor will not allow an insertion point after the image.

I've found this as well, but I don't think it's related to this PR.

@herbdool
Copy link

herbdool commented Sep 2, 2018

With the latest commit the data attribute and thumbnail issues are fixed though I don't know what else needs to be tested.

@quicksketch
Copy link
Member

Things I can think of that need validation:

  • Uploading an image still works.
  • Editing an image that has been uploaded opens the dialog to the upload version, with the value prepopulated so the image can be detached/removed.
  • Selecting an image from the browser can insert an image.
  • Selecting an image from the browser can replace an image.
  • Manually inserting an image (both remote and relative) URL works.
  • Editing an image that is manually inserted continues saving the URL.
  • Copy/paste an image still works.
  • Editing an image that is copy/pasted in should work as though the image were uploaded through the dialog.
  • The pager to go between multiple pages of images works.
  • Images can be inserted after using the pager.

@docwilmot
Copy link
Contributor

Tested all above, including drag and drop, all work normally.
One unexpected finding is that if you paste an image into CKEditor, then doubleclick to edit, the dialog toggles switch position with library toggle to the left of the upload toggle. This persists anytime you doubleclick that particular image, but when editing other images the toggles stay where they were.

FWIW I would not consider this a blocker.

@docwilmot
Copy link
Contributor

But.. Couple of notices in logs:
Notice: Undefined index: callback in ajax_form_callback() (line 394 of /home/qa/github/backdrop/backdrop/pr/2198/core/includes/ajax.inc).

and

Notice: Undefined index: type in filter_format_editor_image_form_submit() (line 332 of /home/qa/github/backdrop/backdrop/pr/2198/core/modules/filter/filter.pages.inc).

@quicksketch
Copy link
Member

One unexpected finding is that if you paste an image into CKEditor, then doubleclick to edit, the dialog toggles switch position with library toggle to the left of the upload toggle.

Fixed this issue.

Notice: Undefined index: type in filter_format_editor_image_form_submit()

Fixed this one too.

I'm not sure about the Undefined index: callback in ajax_form_callback(). I made a fair amount of updates in the latest commit and I can't reproduce it. Perhaps it's been corrected.

The latest version reverts some of the field renaming (from image_library[src] back to attributes[src]), so there should be fewer form changes overall now. I changed the logic that selects the default tab, so now the second tab (Select from Library) may be selected by default. I also added a double-click handler to the images in the library so that images can be selected and inserted all at once (similar to the mechanism for opening the image dialog).

@jenlampton
Copy link
Member

jenlampton commented Sep 3, 2018

I left a comment on the PR.
In short: I love it, and the code looks great!
In long: there are a few things to clean up UX-wise, but those can come in follow-ups.

@quicksketch
Copy link
Member

Super, thanks @jenlampton! With that, I think we've gotten this to a stable and strong state, though possibilities for improvement are still out there.

I've merged backdrop/backdrop#2198 into 1.x for 1.11.0. Thanks everyone for all the excellent feedback. Credit is in backdrop/backdrop@368a744. Thanks especially @Graham-72 for his incredible commitment to keep coming back to this month after month! 🚀

@klonos
Copy link
Member

klonos commented Oct 18, 2018

...follow-up tickets filed 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants