Closed Graham-72 closed 6 years ago
...small UX nit: you know how it gets annoying when you have a toggle that gets moved to a different place once you have clicked on it? Well, when the library was to the right of the controls, this did not happen. Now it that we have moved it to the left, it does happen when you try to toggle between upload vs select from library. That's because when the dialog initially loads, it is aligned to the left of the screen. Switch it to be aligned to the right, and this issue goes away 😉
...small UX nit: you know how it gets annoying when you have a toggle that gets moved to a different place
@klonos I am not fully understanding you because I am always seeing 'image upload' to the left of 'select from library' which is one of the toggles here. Or are you commenting on the fact that the dialogue is moving to the right of the screen?
I think that it would be OK if the image tiles were the same size.
@klonos again I am not understanding the point you are making. Both library displays (this PR and MC) are using fixed-size cells for their thumbnails and reducing the images proportionately to fit within these cells.
Actually imagecache (image styles) does the resizing automatically. It only generates the thumbnail when you try to view it. Say you visit path-to-thumbnail.jpg and it doesnt yet exist, it is then created. So no we wouldnt need to batch convert if we used styles.
@docwilmot thanks for this comment four days ago. If only I could modify the file path token [uri] used by the view in the first of its two global custom text fields by inserting '/styles/thumbnail/public' into the file URI it would use thumbnails rather than the original image files.
After some more work I have now had success with using a template file to modify the first of the two global text fields used in the library view. The template file has the name
The code I have used is
<?php
// $output contains URI of image.
// Find position for insert at the end of '/files'.
$insert_at = strpos( $output, '/files');
// Use a new thumbnail subdirectory.
$add_to_uri = '/styles/thumbnail_150/public';
$new_output = substr_replace ($output, $add_to_uri, $insert_at + 6, 0 );
print ($new_output); ?>
I have added a new image style which scales images to a maximum height of 150px.
No doubt there are more efficient ways of coding this template.
@klonos I am not fully understanding you...
Ok, screenshot time! 😄 ...
So I have intentionally grabbed this ^^ on a 20" monitor. Notice how:
a) the dialog is aligned to the left of the screen when it is loaded. b) when I click on the "Select from library" link, it expands and the controls are not to the right side of the screen. c) my mouse cursor pauses half-way as it travels across the other end of the screen (trying to reach the controls). This is because I have to physically lift my mouse up, move it back, then continue the mouse moving.
Now, if the dialog was popping up to the right of the screen, then image gallery would be the one to expand to the left, and my mouse would still be at the portion of the screen that holds the controls:
...see how I barely need to move my mouse now?
Notice how both portrait and landscape images have equal paddings above/below and left/right?
Well, our version does this only for portrait images, and top-aligns landscape ones (I have hacked the hover state using the browser code inspector in order to make it more obvious):
I find the MC version better because it also adds a subtle gray background color to fill the padding. This makes all the image thumbnails look square, whereas we can only tell that these are tiles when hovered or clicked on.
@klonos thank you for your detailed and very well illustrated explanations to which I respond as follows.
Regarding the style of the image gallery, it seems that in summary there are only two differences you are suggesting (a) for images that do not fill their cell vertically, to equalise the space top and bottom, rather than align their tops. (b) to provide a light background color for every cell as in MC, not just on hover or selection. These are fine adjustments to the CSS which can easily be implemented if this is the concensus.
Regarding the position of the pointer I have a very different view which I will try to explain. First I would point out that although there is a form of toggle action, this is not a true toggle between 'Image library' and 'Select from library' because they are separated physically, as in the Insert Link dialogue where mouse movement is required from one to another. More importantly, if one considers a users sequence of actions, the normal flow is surely having clicked 'Select from library' to click on one of the image thumbnails, and the pointer is already within that screen area. This may be happy coincidence but to me it seems right and there would be no justification in adding to the script in order to move the pointer over to the right.
I have made some more modifications to the PR so that the Image Library is now using thumbnail image files created by a new Image Style I have named 'thumbnail_150' which scales image to a maximum height of 150px for further scaling if necessary by CSS to fit within the image library cell 150px square.
To modify the Library Image view I have added a template file in the templates folder of core's theme Seven. I am not sure that this is the right thing to do, but it works!
@Graham-72 Thanks for the Mailchimp screenshot.
(suggestions by @klonos) : (a) for images that do not fill their cell vertically, to equalise the space top and bottom, rather than align their tops. (b) to provide a light background color for every cell as in MC, not just on hover or selection. These are fine adjustments to the CSS which can easily be implemented if this is the concensus.
Personally, I still prefer the cropped square images because they look more nicely but I see that the 'scaled to fit' approach like in Mailchimp has other advantages. (Still hoping that the respective configuration setting as suggested in https://github.com/backdrop/backdrop-issues/issues/3134#issuecomment-402020604 will be available.) I agree with both of @klonos' suggestions regarding the 'scaled to fit' approach and would like to add another one for a better look and feel: remove the borders from, and add some margin between the 'cells'.
Another topic: testing the PR on the sandbox site I didn't find an easy way to deselect a selected image using the mouse and the keyboard (tried it with click + cmd
on a Mac).
Thanks for comments.
I have been thinking more about this and personally concluded that in order to achieve an aim of making this very familiar to my users, it would be best to make this image library very similar in style and operation (i.e. selection of an image) to what the majority of them are used to, which I suggest is File Explorer in Windows 10 in default style. See this partial screenshot:
The thumbnails have a white background unless hovered over when it becomes pale blue with no border, and a slightly stronger blue with a thin border when selected. There is no provision for 'unselecting' except by selecting a different one. The thumbnail images are proportional to their originals. They are centered horizontally in their grid cells but vertically aligned at the bottom.
This would then be consistent with what they see when selecting and uploading a single image file from their PC, using the Insert Image / Choose file procedure.
I have now revised the PR so that the Image Library View matches the styles seen in Windows 10 File Explorer and also uses the standard thumbnail image style used in Backdrop (100 x 100 px).
making this very familiar to my users, it would be best to make this image library very similar in style and operation (i.e. selection of an image) to what the majority of them are used to, which I suggest is File Explorer in Windows 10 in default style
In my opinion, the Windows style isn't visually attractive. I see the point of many people being used to Windows 10. For the default I however wouldn't choose the design of a specific operating system but the look and feel of the Backdrop user interface. @Graham-72 - what do you think of providing the Windows 10 orientated design as one of several configurations to choose from?
Not that Windows Explorer should be setting the example of best UX (😛...this coming from a Windows user ...that has turned Mac for the sake of work ...that does not particularly like it ...and that secretly wants to switch to Linux when the time is right). Having said that, I do agree that the majority of people globally are using Windows.
For completeness, and in order to get a better idea of commonly-used patterns, here's a screenshot of how things look in Mac Finder:
Hare, also what the file manager in Ubuntu looks like (this one could use a little bit of ❤️TBH):
I wanted to provide screenshots to demo how things work in WP, but at the same time did not want to derail this issue here. So: #3203
@jenlampton I am now stuck on making progress with this issue because of differing views on usability. I think moving the image library to the the left of the screen as you suggested is a big improvement but there are many options for style and differing views on this. I am reluctant to add options for the style of the library because (a) it will take more development time and (b) it results in more code (bloat?).
Is this something your Design and Usabilty meeting might advise on? I just want to achieve a clear and easy-to-use process for when a user selects the 'image' icon. I envisage that in the future there might also be a 'media' icon in the WYSIWYG editor that provides a more comprehensive set of features.
@Graham-72 I'm also of the mind that less is more here, and we should shoot for getting it done over getting it perfect. The current PR and sandbox is so much better than what's in core now, my recommendation would be that we get this in as-is, and then iterate if necessary.
The only significant change may be required is to make the library screen responsive. On smaller screens let's stack the right sidebar below the thumbnails. This may not be the perfect solution, but It's good enough to get it in! (The majority of the time people will be inserting images from a laptop anyway.)
@jenlampton thanks for taking a look at this again. I have now modified the styles so that the screen is responsive and for widths narrower than 1030px the form is displayed below the library grid of thumbnails. I chose this break point because it is where the horizontal scroll would otherwise cut in. I have also made some small changes and additions to wording so that hopefully it is more intelligible to users.
I left one minor suggestion about the view on the PR, but otherwise this looks great to me! Let's get her in and then iterate :)
I also left a comment. I would say though that I really prefer the gallery being on the right side so that the location of the form is consistent when you toggle. Ideally though we need to fix the location of the dialog. It should be centered at all times rather than left-aligned for the upload toggle. Also after toggle it doesnt re-center itself properly till you scroll the whole page and I see a quick 'jump' to the center on scrolling. And a nice-to-have would be animation during the toggle.
Good ideas @docwilmot, can we get them into a follow up issue?
On Sun, Jul 22, 2018, 11:12 PM docwilmot notifications@github.com wrote:
I also left a comment. I would say though that I really prefer the gallery being on the right side so that the location of the form is consistent when you toggle. Ideally though we need to fix the location of the dialog. It should be centered at all times rather than left-aligned for the upload toggle. Also after toggle it doesnt re-center itself properly till you scroll the whole page and I see a quick 'jump' to the center on scrolling. And a nice-to-have would be animation during the toggle.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/backdrop/backdrop-issues/issues/3134#issuecomment-406950124, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYSR3T_bQwxIdkqrEytzrrGeFLWBZP6ks5uJWk7gaJpZM4UFqRC .
I feel that the UX issues with the form placement, and the form jumping around on scroll should be sorted out before we seal it into core. It wouldn't feel right otherwise.
I don't see it as any great problem to implement these changes if we are sure that is what we want to do, though I'm not sure about the animation on toggling.
I left one minor suggestion about the view on the PR.
@jenlampton thank for this suggestion which I have incorporated with an adaptation that enables it to use the thumbnail image files rather than the full sized ones. I am not sure whether I have implemented this in a Backdrop-approved way! I have taken the views handler for the uri field and added an extra checkbox option so that this field can be set to provide the thumbnail url, derived from the main image url. I have added this custom views handler to the Filter module on the assumption that this means it will have no impact on views belonging to other modules.
I have also made some adjustments to the stylesheet for the dialogue form, in the light of some of the comments made.
I left a code review on the PR: https://github.com/backdrop/backdrop/pull/2198#pullrequestreview-143237948
Some issues worth pulling out from the PR:
I also agree with all @docwilmot's suggestions above: put the gallery on the right, center the dialog, and animate the transition.
@quicksketch thanks for the code review. I have made several changes to the PR. External image URLs can now be entered instead of selecting one of the library images. The image style files used for the thumbnails is now selected within the view from a selection list of those available, mainly to ensure that a valid style name is selected. The actual size in the display is controlled by styles in filter.css.
Regarding appearance of the UI, I was strongly influenced by the comment above on 21 June by @jenlampton who wrote
My only thought was that maybe the form fields should be on the right and images on the left, since what I'm most excited about (as a user) was browsing the images, so I wanted to see those first.
I therefore changed them, and prefer the result. However, they can of course be changed back.
For narrower screens they are now centred one above the other but I am strongly of the opinion that on wider screens it is better to make full use of the width and have them alongside each other.
I don't know about animating the transition. How exactly?
@Graham-72 the problem I have with the gallery on the left is just because the entire dialog is not centered. To explain: notice when you first click the image icon, the dialog consists of only the form, and it is stuck on the left of the page. It seems natural then that when you toggle the gallery, that form should stay on the left and the gallery appear on the right. It seems really weird that the form moves so dramatically from entirely on the left side of the page to entirely on the right.
This is why I mentioned that 'ideally' we should center the dialog and animate it. If on first load the dialog (just the upload form) is centered, then on toggling it wouldn't matter much which side the gallery is as it would be obvious that it would no longer be able to stay in the center. If it was animated, it would be visually more satisfying to see it move and return to the center with the toggle.
@docwilmot Thanks for the added explanation. Now I understand, and yes that would be nice.
I can push a change to the PR to animate and center the dialog. I don't think that will take much work, just a CSS transition and triggering the auto-centering code on the tab toggle.
I pushed up a commit to the current PR that animates the dialog. I also found that the data-fid
tag was not being added to images, so I updated the click handler and the view to include the file FID.
The config file views.view.image_library.json
has to be included for new installations to pick up the view, so I exported it and included it in the PR. That means we have the entire view twice: once in a config file for new installations, and once in the .install file to add it to existing sites. That's required in order to make it so that we get consistent starting points when we update the view in the future.
I also flipped the form (again) so that the fields are on the left and the library is on the right. IMO it makes for a smoother transition.
The JS might need additional cleanup, I found it a bit hard to follow, although it seems to work.
There are a few critical issues that I've found while testing:
I rebased the PR at https://github.com/backdrop/backdrop/pull/2198 as it's been open for quite a while. I think I found the AJAX issue with Views: Views was passing in a jQuery object instead of DOM Element for the "context" of the new AJAX objects. I corrected the Views AJAX instantiations but also made it so that jQuery objects will be converted back into DOM elements if necessary.
With that fixed, I updated the View to re-enable AJAX and turn back on the pager. There was also a fair amount of cruft in the view that could be removed, and I removed excess fields that weren't being displayed.
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?
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?
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.
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
.
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.
With the latest commit the data attribute and thumbnail issues are fixed though I don't know what else needs to be tested.
Things I can think of that need validation:
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.
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).
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).
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.
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 https://github.com/backdrop/backdrop/pull/2198 into 1.x for 1.11.0. Thanks everyone for all the excellent feedback. Credit is in https://github.com/backdrop/backdrop/commit/368a744e35362d09bde4c0924ff86293090fd173. Thanks especially @Graham-72 for his incredible commitment to keep coming back to this month after month! :rocket:
...follow-up tickets filed 😉
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: https://github.com/backdrop/backdrop/pull/2198