Show checkbox where the favourite icon is now #6709
Conversation
danxuliu
added
3. to review
| @@ -338,22 +337,23 @@ table td.filename .nametext, .uploadtext, .modified, .column-last>span:first-chi | |||
| /* TODO fix usability bug (accidental file/folder selection) */ | |||
| table td.filename .nametext { | |||
| position: absolute; | |||
There was a problem hiding this comment.
Damn, too many absolute on the filelist! :(
This is a mess!
|
Acceptance failed though! :) |
|
@jancborchardt What do you think? I don’t like this change design-wise and would change the behavior in the recent file listing instead 🤔 |
Thanks :-D
I share your pain :-P
The difference is that some CSS changes that took me hours would have taken you minutes :-P
Mmmm, it works fine on my local machine. I guess that the problem comes from the animation when the right sidebar is opened; when the test finds the element it has certain position, but when it tries to click on it it has moved and now a different element is at that position. I suppose that it could be "fixed" in the same way as other synchronization problems between the test code and the browser: (automatically) trying again :-P But first I have to be able to replicate it... Could you (or anyone else, I can't do it myself as far as I know) set the review to Request changes to prevent the pull request from being merged? This has to be fixed here instead of in a later pull request; otherwise merging this in its current state would cause the acceptance tests to sporadically fail on other pull requests.
Please note that I have changed this because it was agreed in a design session at the conference ;-) |
|
Nice! Just two things:
@MariusBluem as @danxuliu said, this was the result of a proper design discussion and lots of UX confusion, among other things people not knowing how to multiselect stuff, and people being confused because they wanted to open a file/folder via click on the icon/preview but instead it selected. |
Ok, I will fix this as well as the acceptance test issue in the next days.
I would do that in a separate pull request. |
|
Any news? |
Mmm, what opacity should I set for unhovered items? 0.3? 0.5? Other value? Opacity 0.3:
Opacity 0.5:
I have noticed that checkboxes were wrong in the trash bin and fixed them (I will push those changes when I push the rest of them). Regarding the acceptance tests they are almost fixed... but there is still one strange corner case that I am still studying. I hope to finish with that soon ;-) |
|
@danxuliu nice! Let's go for .3 (or .4) - .5 is a bit too much. :) |
danxuliu
force-pushed
the
show-checkbox-where-the-favourite-icon-is-now
branch
from
4f4865a to
266e0bb
Compare
Codecov Report
@@ Coverage Diff @@
## master #6709 +/- ##
============================================
- Coverage 53.04% 52.76% -0.28%
+ Complexity 22690 22688 -2
============================================
Files 1430 1369 -61
Lines 88193 80604 -7589
Branches 1349 0 -1349
============================================
- Hits 46781 42530 -4251
+ Misses 41412 38074 -3338
|
Done; 0.3 then ;-) I have amended the commits to fix the opacity and the trash bin list, and added a new commit to solve the acceptance test issue. Let's wait for Drone's verdict, but they should work now ;-) For reference, I copy here too the commit message explaining how the acceptance tests issue was solved: Firefox and Chrome drivers for Selenium refuse to click on an element if the point to be clicked is covered by a different element, throwing an UnknownError exception with message "Element is not clickable at point ({x}, {y}). Other element would receive the click: {element}". Although in general that would be a legit error (as the user would not be able to click on the element) due to a bad layout, sometimes this can be just a temporal issue caused by an animation, in which case there would be no problem once the animation finished and the elements are all in their final location. Unfortunately, automatically handling those situations in which the problem is caused by an animation by just retrying a few times if the element to be clicked is covered before giving up would probably cause confusion instead of easing test writing. The reason is that if the center of the element is covered by another one the Firefox driver for Selenium tries to click on the corners of the element instead. The problem is that the coordinates used for the click are integer values, but Firefox has sub-pixel accuracy, so sometimes (depending on which corner is not covered and whether the left, top, width or height properties of the element to be clicked have a decimal component or not) the clicks silently land on a different HTML element (and that is with squared borders; with round borders they always land on a different HTML element. That was partially addressed for Selenium 3.0 by clicking first on the edges, but it would still fail if the middle point of the edges is covered but not the corners). It is not possible to fix or even detect all that from the tests (except maybe with some extreme hacks involving accessing private PHP members from Mink and bypassing or replacing the standard JavaScript executed by the Firefox driver with a custom implementation...), so it is not possible to ensure that clicks during an animation will land on the right element (in fact it is not possible even on static elements, although except when the layout is wrong there should be no problem); sometimes retrying a click when the element is covered would solve the problem, sometimes it would cause a different element to be clicked (and sometimes there would be even no retry, as the first click would have silently landed on a different element than the expected one). Therefore, a different approach must be used. Instead of trying to automatically handle clicks during animations the tests must be written being aware of the problem and thus waiting somehow for the animations that can cause a problem to end before performing the clicks. |
🤦 I was so focused on the acceptance tests that I forgot to run the unit tests... I will fix them and push again ;-) |
Icon class function properties make possible to render a different icon class depending on the context of the file action. Inline file actions had support for them already and called them passing the file name and context of the file action as parameters. Due to this the FileActionsMenu passes those parameters too to icon class functions instead of just the context like done for display name functions. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
The new FileAction for the menu is essentially the same as the old inline FileAction, except for the rendering; in this case the FileAction is shown in the menu in a standard way, so there is no need to provide a custom renderer (although the menu entry text and icon change depending on whether the file is currently a favorite or not, but that can be done just with displayName and iconClass functions). Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Currently a file can be favorited either through the inline action or through the file actions menu. However, the inline action will be removed in a following commit and then it will be possible to do it only through the file actions menu. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
This is a preparatory step for a following commit in which the position of the favorite icon and the checkbox will be swapped; in that new design the favorite icon is no longer expected to be an action but just a simple mark on whether the file is favorited or not (the action is expected to be triggered then only from the file actions menu). The favorite icon is now fully shown or completely hidden depending on whether the file is favorited or not. As the icon is just informative but no longer an action now it does not change when hovered or focus. In the same way, the alternative text when the file is not favorited now it is not "Favorite" (an action) but "Not favorited" instead. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
The checkbox is not shown always with full opacity, though, in order to reduce the visual noise (specially later, once the checkbox is moved to its own column). Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
The favorite icon was shown on its own "column" (not a real column in the table, but a visual column achieved through margins and left positions). Now the icon was moved to the top right corner of the file thumbnail, and the thumbnail and file name were moved to the left to fill the space left by the "column". To keep the markup in line with its visual representation (and to ease the placing through CSS), the favorite mark is no longer prepended to the row, but appended to the thumbnail instead. In the same way, the thumbnail is no longer appended to the checkbox label, but to the link with the name of the file instead (although the checkbox is still shown at the bottom right corner of the thumbnail, and clicking on the thumbnail still selects the file). In order to show the "busy" state on a file the "icon-loading-small" CSS class is set to the parent element of the thumbnail, so the thumbnail is also wrapped now by another div with the same size and position as the label. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
The "has-favorites" CSS class is no longer used after moving the favorite mark to the top right corner of the thumbnail, so there is no need to add it to the table. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
The selection column is not only a visual column, but also a real column of the file list table. Unlike other columns whose width is reduced in space constrained screens the selection column must stay the same so the tapping area is large enough to be easily usable The selection column does not appear in the search results table, so its contents have to be explicitly aligned with those of the main table based on whether the main table has a selection column or not (using the "has-selection" CSS class in the same way as the "has-favorite" CSS class was being used when there was a column for favorite actions). In the tests the ":visible" selector can no longer be used. That selector matches elements with a width or height that is greater than zero, but the dimensions calculated in the unit tests are not reliable; the width of the link was zero before these changes, and now moving the checkbox to its own column causes the height of the link to become zero too, so it no longer matches the ":visible" selector even if it is not hidden. As hidding and showing the link is based on its "display" CSS property its value is the one checked now. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Now that the checkbox was moved to its own column clicking on the thumbnail should behave like clicking on the file name. To achieve this the left position was replaced with a padding, so the element is kept at the same place while extending its clickable area to cover the thumbnail. Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Firefox and Chrome drivers for Selenium refuse to click on an element if
the point to be clicked is covered by a different element, throwing an
UnknownError exception with message "Element is not clickable at point
({x}, {y}). Other element would receive the click: {element}". Although
in general that would be a legit error (as the user would not be able to
click on the element) due to a bad layout, sometimes this can be just a
temporal issue caused by an animation, in which case there would be no
problem once the animation finished and the elements are all in their
final location.
Unfortunately, automatically handling those situations in which the
problem is caused by an animation by just retrying a few times if the
element to be clicked is covered before giving up would probably cause
confusion instead of easing test writing.
The reason is that if the center of the element is covered by another
one the Firefox driver for Selenium tries to click on the corners of the
element instead. The problem is that the coordinates used for the click
are integer values, but Firefox has sub-pixel accuracy, so sometimes
(depending on which corner is not covered and whether the left, top,
width or height properties of the element to be clicked have a decimal
component or not) the clicks silently land on a different HTML element
(and that is with squared borders; with round borders they always land
on a different HTML element. That was partially addressed for Selenium
3.0 by clicking first on the edges, but it would still fail if the
middle point of the edges is covered but not the corners).
It is not possible to fix or even detect all that from the tests (except
maybe with some extreme hacks involving accessing private PHP members
from Mink and bypassing or replacing the standard JavaScript executed by
the Firefox driver with a custom implementation...), so it is not
possible to ensure that clicks during an animation will land on the
right element (in fact it is not possible even on static elements,
although except when the layout is wrong there should be no problem);
sometimes retrying a click when the element is covered would solve the
problem, sometimes it would cause a different element to be clicked (and
sometimes there would be even no retry, as the first click would have
silently landed on a different element than the expected one).
Therefore, a different approach must be used. Instead of trying to
automatically handle clicks during animations the tests must be written
being aware of the problem and thus waiting somehow for the animations
that can cause a problem to end before performing the clicks.
Signed-off-by: Daniel Calviño Sánchez <[email protected]>
danxuliu
force-pushed
the
show-checkbox-where-the-favourite-icon-is-now
branch
from
266e0bb to
065ab6b
Compare
|
Unit tests fixed and branch rebased onto current master. Drone failure is not related as far as I know. Ready for review again! :-) |
| // As it is currently not possible to provide a context for | ||
| // the i18n strings "Add to favorites" was used instead of | ||
| // "Favorite" to remove the ambiguity between verb and noun | ||
| // when it is translated. |
There was a problem hiding this comment.
It is: just add a comment before the string with:
// TRANSLATORS: ...
See
server/apps/encryption/templates/altmail.php
Lines 10 to 11 in b4f36d4
There was a problem hiding this comment.
I did not know about that, thanks for the info :-)
|
Nice work @danxuliu!! 🎉 |
|
Yup, great stuff @danxuliu! :) Good to have you on Nextcloud |
|
Thanks a lot! :-D |
|
Ah @danxuliu just one thing, is it possible we show the »Add to favorites« / remove action first in the file actions popover? |
Sure, here you have: #7012 :-) |
|
I like the idea of the checkbox being in front on a single column. |
|
cc @jancborchardt ^ |
|
We could show both columns on big displays, however it's just too much info at the same time then. The focus on the files itself gets lost. We dicussed this decision over a long time and came to the conclusion that multiselect should be more present than favoriting since it is done more frequently. Favoriting for example can also be done in the sidebar, whereas multiselect can only be done in the list. And we do the same with the 3-dot-menu in the Android app. So yeah, it was a tough decision. But if everything is on the forefront and important, then nothing is really important and all just gets drowned out. cc @nextcloud/designers |
Fixes #6361
The favourite icon is now just a mark shown on the thumbnail of favourited files (before it was also shown for not favourited files with a different colour). The center of the star is slightly below and to the left of the top right corner of the thumbnail instead of exactly at the corner because, at least for me, it looked better that way ;-) To get the right placement of the thumbnail, the favourite mark, the file text and so on I used a lot of element nesting; I do not know if that could noticeably slow down the rendering on very large file lists or not, but it may be something to keep an eye on ;-)
Marking a file as favourite or unmarking it is done now through the file actions menu (the three-dots menu). Unlike other actions in the file actions menu, the favourite action is a toggle; its text (and icon) change between Remove from favorites and Add to favorites depending on whether the file is already favourited or not (those texts where used instead of the shorter Unfavorite and Favorite due to the lack of translation contexts). The action was placed in the file actions menu between Move or copy and Download (for no special reason, to be honest).
Regarding the checkbox used to select files it was moved visually to the place where the favourite icon was before. In HTML and CSS, however, it was done by prepending a new column to those tables that allow selections; it was easier (at least, for me :P ) with respect to the CSS (as the CSS for the name column relies a lot in floats and absolute positions (just for the record, it was that way before I touched the code :-P), so accomodating a new element inside that column that sometimes had to be shown and sometimes not was difficult, and in fact for the favourite icon it was done by applying margins and left positions when the table had the CSS class
has-favorites), and conceptually it also seemed like using an explicit column in the table to provide the selection was the most logical approach. Given that the checkbox is no longer placed on the thumbnail now clicking on the thumbnail opens the file, just like clicking on the file name (as requested in #6361 (comment)).The aforementioned
has-favoritesclass was used before also to align the results table of the search with the main table; as the results table did not include the favourite action but the main table could have it or not the results table looked for thehas-favoritesclass and, if it was found, it moved its content to the left as needed. The same approach is used now with thehas-selectionclass. Also, the horizontal padding of#searchresults tdwas reduced from 19px to 14px (could this affect other apps?) to align the file names of the results with those of the main table (note that before these changes the space between the thumbnail and the file name in the file list changed slightly depending on whether the table had favourites or not, and that the file names in the results were aligned to the file names in tables with favourites, so the results were not perfectly aligned to tables without favourites; now there is always the same distance between thumbnail and file name, and the results are (or should be :-P ) always perfectly aligned :-) ).Before, all files:
After moving the icon to the thumbnail, all files:
After moving the checkbox to its own column, all files:
Before, recents:
After, recents:
Before, favorites:
After, favorites:
Before, all files with search:
After moving the icon to the thumbnail, all files with search:
After moving the checkbox to its own column, all files with search:
When taking these screenshots I found that search results are shown inconsistently (even on the stable12 branch) depending on whether you refresh a section (All files, Recents...) and then search or you come from a different section and then search (sometimes only the results were shown, sometimes the contents of the folder were shown along with the results, sometimes only the header of the table for the contents of the folder was shown along with the results). @jancborchardt when you have some time could you check that and open an issue detailing what should be the expected behaviour (as you know that better than me ;-) )? Thanks :-)
Back to the checkbox and favourites, on narrow screens keeping the checkbox on the thumbnail could be a better approach than having an exclusive column for the checkbox in order to save valuable horizontal space. However, this would bring back the original problem of selection not being intuitive, specially on touch based devices like mobile phones where it is not natural to hover over the elements; of course the file could be selected just by tapping on the thumbnail, but hovering was what revealed the checkbox and conveyed that it was selectable. Instead of only showing the checkboxes on hover, though, it could be enough to always show the checkboxes to more or less fix the discoverability problem (see the After moving the icon to the thumbnail, all files screenshot above).
I leave that as something to be considered by @nextcloud/designers. In any case, if implemented it will probably be better done on a different pull request. And it will surely have to be done by someone else than me, because I already struggled too much and for too long with the CSS in this pull request and thinking of further changes makes me dizzy :-P
And now I summon @jancborchardt and @skjnldsv for review and, probably, CSS fixes ;-) Thanks!