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

Add confirmation button to share input #7196

Merged
merged 17 commits into from
Mar 21, 2018
Merged

Conversation

jancborchardt
Copy link
Member

Another one in our quest to have a Confirm function for text fields #1068

Needs some actual JS to work @nickvergessen @oparoz @MorrisJobke

@nickvergessen
Copy link
Member

Sample screenshots?

@jancborchardt
Copy link
Member Author

Before:
screenshot from 2017-11-16 22-19-08

After – with confirm button icon:
screenshot from 2017-11-16 22-18-23

As said, needs JS help to work @nickvergessen @MorrisJobke.

@MorrisJobke
Copy link
Member

@danxuliu @nextcloud/javascript Anybody up to make this happen for 13? Otherwise we need to move this to 14.

@MorrisJobke MorrisJobke mentioned this pull request Dec 8, 2017
28 tasks
@rullzer rullzer modified the milestones: Nextcloud 13, Nextcloud 14 Dec 11, 2017
@jancborchardt
Copy link
Member Author

(Just assumed this was requested by a customer too? @oparoz?)

@oparoz
Copy link
Member

oparoz commented Dec 11, 2017

@danxuliu has already developed the code, so this should be merged in 13

@MorrisJobke
Copy link
Member

@danxuliu Could you show me the code? Because I would like to have this in the beta 2 then.

@MorrisJobke
Copy link
Member

As this is not yet ready as discussed with @danxuliu, I will move this to 14 and we can look into how (if at all) can backport this to stable13 later.

@rullzer
Copy link
Member

rullzer commented Feb 8, 2018

So it is time to shine then I guess!

@danxuliu danxuliu added 3. to review Waiting for reviews and removed 2. developing Work in progress help wanted labels Mar 20, 2018
@danxuliu
Copy link
Member

Ready for review!

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 some related unit tests were also improved, as well as certain behaviours (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

@@ -22,18 +22,14 @@
'<div class="oneline">' +
' <input id="shareWith-{{cid}}" class="shareWithField" type="text" placeholder="{{sharePlaceholder}}" />' +
' <span class="shareWithLoading icon-loading-small hidden"></span>'+
'{{{shareInfo}}}' +
' <span class="shareWithConfirm icon icon-confirm"></span>' +
Copy link
Member

Choose a reason for hiding this comment

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

Please use our latest standard: input + input[type='submit'].icon-confirm :)
You will be able to remove the custom css as well :)

Copy link
Member

Choose a reason for hiding this comment

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

@skjnldsv I have been trying to use that, but... it does not work as expected (it shows some accessibility text instead of just the icon, and the position and size is not right either). Could you please take a look? :-)

Copy link
Member

Choose a reason for hiding this comment

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

Of course! :)
You need to define an empty value to override html default behaviour :)

Copy link
Member

Choose a reason for hiding this comment

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

Okay, did some tests, but the tooltip errors are inserting themselves between the input and the icon and break the flow. Though it's only when an error is displayed, so we could hide the submit button anyway since you can't submit with an error! Let me push!

Copy link
Member

Choose a reason for hiding this comment

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

Nah, too much edits to do! Leave it like you did! We'll work on it when we will standardise the sidebar! :)

jancborchardt and others added 13 commits March 20, 2018 19:09
The confirmation button right now is just an icon; its behaviour will be
added in the following commits.

Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
As the server response is faked the search term is ignored in the tests.
However, it is clearer to use a search term that would make the server
return what the faked response contains.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Stubs should be restored outside the test method in which they are used
to ensure that they are properly restored no matter the result of the
test (for example, if an exception is thrown).

Besides that, this will make possible to reuse the stub in other sibling
tests without having to explicitly setup it in them.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
"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 <danxuliu@gmail.com>
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 <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
"_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 <danxuliu@gmail.com>
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 <danxuliu@gmail.com>
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 the
autocomplete dropdown is shown with the old suggestions. Strange, but
possible nevertheless ;-)

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@MorrisJobke
Copy link
Member

@danxuliu I'm always impressed by your dedication to tests - I simply ❤️ it - 👍

bildschirmfoto 2018-03-20 um 22 46 07

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 <danxuliu@gmail.com>
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 <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
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 <danxuliu@gmail.com>
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 (even in IE11

@MorrisJobke MorrisJobke merged commit 6f455d5 into master Mar 21, 2018
@MorrisJobke MorrisJobke deleted the sharing-confirm-button branch March 21, 2018 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews design Design, UI, UX, etc. enhancement feature: sharing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants