Public link share cleanup #29721

felixheidecke · 2017-11-30T09:42:55Z

Description

Reduce length of date field for external link
Change button text 'Save' to 'Share'
Change mail error text
Add send confirmation after sending notification

Related Issue

Fixes: #29694

How Has This Been Tested?

Visual tests in latest Chrome & Firefox

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests pass.

phil-davis · 2017-11-30T16:20:09Z

@felixheidecke I saw that the UI tests were failing here, due to the text of the button on the UI changing to "Share". I made the change and intended to push it to a separate branch and do a PR to here - but accidentally committed it directly here.

Anyway, the latest commit here should make the UI tests pass.

felixheidecke · 2017-12-01T09:23:53Z

@phil-davis I don't mind you committing directly if it helps the PR pass :-)

DeepDiver1975

please squash commits to reasonable units - THX

PVince81 · 2017-12-11T15:52:13Z

core/js/sharedialoglinkshareview.js

@@ -314,7 +311,7 @@
 			var self = this;
 			var title = t('files_sharing', 'Edit link share: {name}', {name: this.itemModel.getFileInfo().getFullPath()});
 			var buttons = [{
-				text: t('core', 'Save'),
+				text: t('core', 'Share'),


does "Share" also make sense when editing a link ? I'd rather have "Save" on edit and "Share" a creation time

Yes, good point!

felixheidecke · 2017-12-13T09:45:42Z

please squash commits to reasonable units - THX

@DeepDiver1975 doesn't "Squash and merge" do this in one go?

codecov · 2017-12-13T10:02:08Z

Codecov Report

Merging #29721 into master will increase coverage by 2.08%.
The diff coverage is 0%.

@@             Coverage Diff              @@
##             master   #29721      +/-   ##
============================================
+ Coverage     60.52%   62.61%   +2.08%     
+ Complexity    18380    17589     -791     
============================================
  Files          1090     1038      -52     
  Lines         60949    57930    -3019     
============================================
- Hits          36888    36271     -617     
+ Misses        24061    21659    -2402

Impacted Files	Coverage Δ	Complexity Δ
core/ajax/share.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
apps/dav/lib/Files/FilesHome.php	`0% <0%> (-100%)`	`4% <0%> (-8%)`
lib/private/Template/Base.php	`71.11% <0%> (-2.48%)`	`13% <0%> (-4%)`
lib/private/L10N/Factory.php	`92.98% <0%> (-2.48%)`	`63% <0%> (-3%)`
lib/private/legacy/app.php	`58.33% <0%> (-1.16%)`	`202% <0%> (+5%)`
lib/private/Route/Route.php	`74.41% <0%> (-1.14%)`	`14% <0%> (ø)`
apps/dav/lib/Server.php	`48.33% <0%> (-0.85%)`	`11% <0%> (-1%)`
lib/private/legacy/template.php	`43.94% <0%> (-0.71%)`	`38% <0%> (-1%)`
lib/private/Server.php	`82.18% <0%> (-0.46%)`	`128% <0%> (ø)`
lib/private/User/Manager.php	`75.75% <0%> (-0.27%)`	`59% <0%> (-1%)`
... and 61 more

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 25ba394...bbd325d. Read the comment docs.

phil-davis · 2017-12-13T11:30:24Z

@felixheidecke "Squash and Merge" squashes everything into a single commit. It has been disabled on this repo, because (for generating release notes changelog etc...) the ordinary "Merge pull request" is useful. It makes a "merge commit" on top of the "real" commit(s). That merge commit has a reference back to the PR and makes automated extraction of changes back to PRs easier. (my understanding at least)

It's done

davitol · 2017-12-14T08:40:02Z

@pmaier1 @felixheidecke

In the ticket related #29694 it is said:

Adapt password and expiration date fields to a reasonable length
Currently these fields use the whole width of the modal. Just give them a reasonable length to make it look better (e.g. the date field always has the same width). For consistency I suggest to make the length of the two fields equal.

but with this PR we get this:

davitol · 2017-12-14T09:39:21Z

@pmaier1 @felixheidecke
Show "Sending"/"E-Mail sent" indicator longer and maybe turn green upon success
Currently the modal disappears in, say, 0,1 seconds after clicking the button so users have a hard time to see if the action was successful or not (I guess there is no "unsuccessful" but users should be able to take note of the success). Please increase the time the modal is shown after success to a reasonable value, e.g. 1-2 sec.

The solution given is a pop up that appears after sending the email

pmaier1 · 2017-12-14T09:42:14Z

For consistency I suggest to make the length of the two fields equal.

Hmm, please adapt..

The solution given is a pop up that appears after sending the email

From #29694:

Please increase the time the modal is shown after success to a reasonable value, e.g. 1-2 sec.

davitol · 2017-12-14T10:00:24Z

❌ Adapt password and expiration date fields to a reasonable length
Currently these fields use the whole width of the modal. Just give them a reasonable length to make it look better (e.g. the date field always has the same width). For consistency I suggest to make the length of the two fields equal.
Rename the "Save" button to "Share"
This is to better reflect that the share and notifications effectively are created when the button is used.
Rename error message when invalid mail addresses are entered
=> "Could not send mail to the following recipient(s)"
This is to better differentiate between external recipients and internal users
❌ Show "Sending"/"E-Mail sent" indicator longer and maybe turn green upon success
Currently the modal disappears in, say, 0,1 seconds after clicking the button so users have a hard time to see if the action was successful or not (I guess there is no "unsuccessful" but users should be able to take note of the success). Please increase the time the modal is shown after success to a reasonable value, e.g. 1-2 sec.

Tested in Firefox, Chrome, Safari, Edge and IE11 browsers

felixheidecke · 2017-12-14T12:32:13Z

As seen in the screenshot … the wording is rather inconsistent. I'm sure there are no guidelines on wording for forms; @pmaier1 pls find the right words for title and placeholder so I can choose a length.
Also, this length can differ in other languages … so giving a bit more space is recommended.

felixheidecke · 2017-12-14T12:40:03Z

… maybe turn green …

While "solutions" are made up on the spot and case by case, we keep creating an inconsistent user experience. Nowhere in ownCloud, we have this type of UI feedback so I wouldn't recommend it introducing it here.

While I am currently working on bringing UIKit into the game and creating a unified and consistent UI/UX, I suggest sticking to the separate "notification was sent to xyz" for now.

pmaier1 · 2017-12-14T16:14:21Z

@pmaier1 pls find the right words for title and placeholder

Suggestion

Password
Choose a password.

Expiration date
Choose an expiration date.

and align text field length

Also, this length can differ in other languages … so giving a bit more space is recommended.

Good point!

While "solutions" are made up on the spot and case by case, we keep creating an inconsistent user experience. Nowhere in ownCloud, we have this type of UI feedback so I wouldn't recommend it introducing it here.

Turning green was just an idea. What's required here is just that the modal stays visible a bit longer. It already says something like "sent" or similar.

Change button text 'Save' to 'Share' Change mail error text Add send confirmation after sending notification defer link creation upon failing email notification Modify UI tests for Public Link dialog Share button Show button text "share" if creating a new share

lock · 2019-08-01T15:02:30Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

felixheidecke added enhancement feature:sharing p2-high Escalation, on top of current planning, release blocker labels Nov 30, 2017

felixheidecke added this to the development milestone Nov 30, 2017

felixheidecke requested review from PVince81, davitol and pmaier1 November 30, 2017 09:42

ownclouders added the review carefully! label Nov 30, 2017

felixheidecke added the 3 - To Review label Nov 30, 2017

pmaier1 removed their request for review November 30, 2017 10:57

owncloud deleted a comment from codecov bot Dec 1, 2017

felixheidecke mentioned this pull request Dec 1, 2017

Email validation in public sharing dialog doesn't have a good UX #29703

Closed

felixheidecke added in progress and removed 3 - To Review review carefully! labels Dec 1, 2017

ownclouders added the review carefully! label Dec 1, 2017

felixheidecke force-pushed the public-link-share-cleanup branch 2 times, most recently from 2446121 to 4a28344 Compare December 1, 2017 12:07

owncloud deleted a comment from codecov bot Dec 4, 2017

DeepDiver1975 added 2 - Developing and removed in progress labels Dec 6, 2017

DeepDiver1975 assigned felixheidecke Dec 6, 2017

DeepDiver1975 previously requested changes Dec 6, 2017

View reviewed changes

PVince81 reviewed Dec 11, 2017

View reviewed changes

felixheidecke force-pushed the public-link-share-cleanup branch 2 times, most recently from 505f118 to 90b08dc Compare December 13, 2017 09:37

owncloud deleted a comment from codecov bot Dec 13, 2017

felixheidecke force-pushed the public-link-share-cleanup branch from 90b08dc to 1f90b6b Compare December 13, 2017 10:01

felixheidecke force-pushed the public-link-share-cleanup branch 6 times, most recently from 9e4bbb2 to c849f17 Compare December 13, 2017 14:11

felixheidecke force-pushed the public-link-share-cleanup branch from c849f17 to a874a38 Compare December 14, 2017 12:45

felixheidecke force-pushed the public-link-share-cleanup branch from a874a38 to bbd325d Compare December 15, 2017 13:39

felixheidecke mentioned this pull request Dec 20, 2017

Share personal cleanup + note #29935

Merged

9 tasks

felixheidecke closed this Dec 20, 2017

felixheidecke deleted the public-link-share-cleanup branch December 20, 2017 15:07

lock bot locked as resolved and limited conversation to collaborators Aug 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Public link share cleanup #29721

Public link share cleanup #29721

felixheidecke commented Nov 30, 2017 •

edited

Loading

phil-davis commented Nov 30, 2017

felixheidecke commented Dec 1, 2017

DeepDiver1975 left a comment

PVince81 Dec 11, 2017

pmaier1 Dec 11, 2017

felixheidecke Dec 13, 2017

felixheidecke commented Dec 13, 2017

codecov bot commented Dec 13, 2017 •

edited

Loading

phil-davis commented Dec 13, 2017

davitol commented Dec 14, 2017 •

edited

Loading

davitol commented Dec 14, 2017

pmaier1 commented Dec 14, 2017

davitol commented Dec 14, 2017 •

edited

Loading

felixheidecke commented Dec 14, 2017

felixheidecke commented Dec 14, 2017

pmaier1 commented Dec 14, 2017 •

edited

Loading

lock bot commented Aug 1, 2019

Public link share cleanup #29721

Public link share cleanup #29721

Conversation

felixheidecke commented Nov 30, 2017 • edited Loading

Description

Related Issue

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

phil-davis commented Nov 30, 2017

felixheidecke commented Dec 1, 2017

DeepDiver1975 left a comment

Choose a reason for hiding this comment

PVince81 Dec 11, 2017

Choose a reason for hiding this comment

pmaier1 Dec 11, 2017

Choose a reason for hiding this comment

felixheidecke Dec 13, 2017

Choose a reason for hiding this comment

felixheidecke commented Dec 13, 2017

codecov bot commented Dec 13, 2017 • edited Loading

Codecov Report

phil-davis commented Dec 13, 2017

davitol commented Dec 14, 2017 • edited Loading

davitol commented Dec 14, 2017

pmaier1 commented Dec 14, 2017

davitol commented Dec 14, 2017 • edited Loading

felixheidecke commented Dec 14, 2017

felixheidecke commented Dec 14, 2017

pmaier1 commented Dec 14, 2017 • edited Loading

Suggestion

lock bot commented Aug 1, 2019

felixheidecke commented Nov 30, 2017 •

edited

Loading

codecov bot commented Dec 13, 2017 •

edited

Loading

davitol commented Dec 14, 2017 •

edited

Loading

davitol commented Dec 14, 2017 •

edited

Loading

pmaier1 commented Dec 14, 2017 •

edited

Loading