Skip to content
This repository was archived by the owner on Nov 1, 2020. It is now read-only.

Conversation

@jancborchardt
Copy link
Member

The Gallery app equivalent of nextcloud/server#7196 cc @nickvergessen @oparoz @MorrisJobke

@jancborchardt jancborchardt added 2. developing Work in progress backport-request Pending backport by the backport-bot coder wanted design Related to the design labels Nov 16, 2017
@jancborchardt jancborchardt added this to the Nextcloud 13 milestone Nov 16, 2017
@MorrisJobke
Copy link
Member

Moved to 14 (same as the server PR)

@danxuliu danxuliu force-pushed the sharing-confirm-button branch from 1f1e6a7 to 7a5f175 Compare March 26, 2018 04:51
@danxuliu
Copy link
Member

Ready for review!

I have adapted the commits from nextcloud/server#7196 to the gallery code (although there are no unit tests in this case, sorry ;-) ). Note, however, that things like avatars or not showing sharees already shared with in the dropdown (which are implemented in the server, but not in the gallery) were not included; only the changes related to the confirmation button were taken into account.

Copy-pasting myself:
Now a share is added when clicking on the confirm button of the input field (or when pressing Enter on the input field). However, that only happens if the server returns a single exact match for that sharee; if no exact match is found for the input field value, or if there are more than one, no share is added and the autocomplete dropdown is shown again with the suggestions. I think that behaviour is less error prone for the user than simply adding a share with the first match found (and if the user wants to do that she just needs to wait until the autocomplete dropdown is shown and either click on the first item or simply press Enter (as the first item is focused automatically when the dropdown is shown)).

In this pull request the order of suggestions in the autocomplete dropdown has been slightly modified. Until now the order was exact users, partial users, exact groups, partial groups, exact...; now that order became exact users, exact groups, exact..., partial users, partial groups, partial..., which in my opinion is better as all the exact matches are now shown at the beginning.

Besides that certain behaviours were also improved (like no longer failing silently when getting the suggestions succeeded but failure content was returned (for example, if OCSBadRequestException was thrown in the controller), or keep showing the working icon while there are pending operations) and some minor details in the code (using OC.Notification.showTemporary instead of OC.Notification.show plus OC.Notification.hide after a timeout).

@nextcloud/designers

@danxuliu danxuliu added enhancement New feature or request 3. to review Waiting for reviews and removed 2. developing Work in progress coder wanted labels Mar 26, 2018
@danxuliu
Copy link
Member

Please wait until #421 is merged to merge this one (it will need a rebase).

@MorrisJobke
Copy link
Member

Please wait until #421 is merged to merge this one (it will need a rebase).

Please rebase as #421 was merged.

jancborchardt and others added 11 commits April 3, 2018 23:45
The confirmation button right now is just an icon; its behaviour will be
added in the following commits.

Signed-off-by: Jan-Christoph Borchardt <[email protected]>
"OC.Notification.hide" expects the notification to be hidden to be
passed as an argument. As it was being used to show a temporary
notification the combination of "OC.Notification.show" and
"OC.Notification.hide" was replaced by a single call to
"OC.Notification.showTemporary".

The timeout could have been specified in the options of the call, but it
was left to the default value (7 seconds) for consistency with other
notifications.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Instead of silently failing now an error is shown to the user when the
ajax call to get the suggestions succeeds yet it returns failure content
(for example, if an "OCSBadRequestException" was thrown in the server).

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
"_getSuggestions" returned all the suggestions from the server, which
are composed by exact matches and partial matches. Now the exact matches
are also returned on their own parameter. This will be used by the
button to confirm a share.

Note that until now the order of the suggestions was "exact users,
partial users, exact groups, partial groups, exact..."; this commit also
changes that order to become "exact users, exact groups, exact...,
partial users, partial groups, partial...". This is not a problem, as
the suggestions were used in the autocomplete dropdown, and this new
order is arguably better than the old one, as all exact matches appear
now at the beginning.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
This unifies the behaviour with the one used in the file list, and it
will also simplify other changes in following commits (like not needing
to show error messages when confirming a share and instead rely on those
from the autocompletion).

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Clicking on the confirm button now adds a share, but only if there is
just a single exact match. If there are no exact matches or there is
more than one exact match no share is added, and the autocomplete
dropdown is shown again with all the suggestions.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Besides confirming a share by clicking on the confirm button now it is
possible to do it by pressing enter on the input field.

Clicking on the confirm button implicitly hides the autocomplete
dropdown. On the other hand, pressing enter on the input field does not,
so the autocompletion must be disabled and closed when the confirmation
begins and then enabled again once it finishes. Otherwise the
autocomplete dropdown would be visible and it would be possible to
interact with it while the share is being confirmed.

The order in which the input field and the autompletion are disabled is
important. Internally, the autocompletion sets a timeout when the input
field is modified that requests the suggestions to the server and then
shows them in the dropdown. That timeout is not cancelled when the
autocompletion is disabled, but when the input field loses its focus and
the autocompletion is not disabled. Therefore, the input field has to be
disabled (which causes it to lose the focus*) before the autocompletion
is disabled. Otherwise it could happen that while a share is being
confirmed the timeout ends, so an autocompletion request is sent and
then, once the share is successfully confirmed and thus the
autocompletion is enabled again, the request is received and processed.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
When a share is confirmed the suggestions are got to check if there is
an exact match. Usually the suggestions were already got with the same
parameters in order to fill the autocomplete dropdown, so to avoid a
superfluous request now the last suggestions are reused when got again,
although only if the same parameters as the last time are used.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
danxuliu added 3 commits April 3, 2018 23:47
The suggestions depend on the results returned by the server, but also
on the sharees already shared with. Due to that adding a share changes
the suggestions, so now the cached suggestions are discarded when a
share is added.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
Before, whenever a pending operation (getting the suggestions,
confirming a share or selecting a recipient) finished the working icon
was hidden and the confirm button was shown again, even if there were
other pending operations (the most common case is typing slowly on the
input field, as several operations to get the suggestions could pile if
the server response is not received fast enough). Now, the working icon
is not hidden until the last pending operation finishes.

Signed-off-by: Daniel Calviño Sánchez <[email protected]>
@danxuliu danxuliu force-pushed the sharing-confirm-button branch from 7a5f175 to a010c69 Compare April 3, 2018 21:54
@codecov
Copy link

codecov bot commented Apr 3, 2018

Codecov Report

Merging #326 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #326   +/-   ##
=========================================
  Coverage     82.55%   82.55%           
  Complexity      360      360           
=========================================
  Files            38       38           
  Lines          1313     1313           
=========================================
  Hits           1084     1084           
  Misses          229      229

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd99aac...a010c69. Read the comment docs.

@danxuliu
Copy link
Member

danxuliu commented Apr 3, 2018

Please rebase as #421 was merged.

Done!

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and works 👍

@MorrisJobke MorrisJobke merged commit bd8226a into master Apr 4, 2018
@MorrisJobke MorrisJobke deleted the sharing-confirm-button branch April 4, 2018 08:11
@danxuliu danxuliu removed the backport-request Pending backport by the backport-bot label Apr 5, 2018
@danxuliu
Copy link
Member

danxuliu commented Apr 5, 2018

As the equivalent pull request in server has not been backported this one should not be backported either to keep a consistent behaviour in Nextcloud 13; due to that I have removed the backport-request label.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

3. to review Waiting for reviews design Related to the design enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants