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

Multiple link shares and UI adjustments for share dialog #5695

Merged
merged 4 commits into from
Apr 21, 2017

Conversation

ckamm
Copy link
Contributor

@ckamm ckamm commented Apr 11, 2017

See #5655

For servers that support it:
screen1

For older servers:
screen2

@pmaier1
Copy link

pmaier1 commented Apr 11, 2017

Great work! Fine for me as it is.

Copy link
Contributor

@SamuAlfageme SamuAlfageme left a comment

Choose a reason for hiding this comment

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

This is a great PR, I like the new design of the sharing dialog a lot! 😊

There's some thoughts result of testing this for a bit now:

  • Server sets the name of the file as default name of the public link.
  • Links can be created in the web UI with no-name: some users might want just to create an old-school link share and now they're forced to set an explicit name.
  • The "Create" button is enabled even when it does nothing, related with the previous points; maybe we can create a link with no explicit name and display the name from the "token" field on the response like the server does? (Also note that the server uses this value to list the public links when no name was set; the new dialog shows "Public link")
  • When clicking a link on the list and "Password protect"/"Set expiration date" do update the link's properties but the name field still applies only for creating new links (update the textbox on click and change the button label to "Update" instead of "Create"?)
  • Also, updating the link name by directly clicking its name on the list does not perform the PUT request to update the name on the server.
  • Actions for each link are stacked in a drop menu (open, copy, send link), when there's enough room for them in the list view
  • The detailed permissions are still shown on the "Users and Groups" tab (ref. ckamm@3d92c2c)

screenshot 2017-04-12 09 58 22

Again, this is an amazing change! 🎉

@ckamm
Copy link
Contributor Author

ckamm commented Apr 12, 2017

@SamuAlfageme Thanks for checking it out, here are detail comments.

Server sets the name of the file as default name of the public link.

I didn't do this because then the nice help text in the edit box typically isn't visible. We could have a label and pre-fill the edit box as suggested. I'll try that.

Links can be created in the web UI with no-name: some users might want just to create an old-school link share and now they're forced to set an explicit name.

Why would someone go "I absolutely want a nameless link share!"? I doubt this is a real use-case and think having a straightforward default-name as you suggested above is sufficient.

The "Create" button is enabled even when it does nothing

Will fix.

display the name from the "token" field on the response like the server does? (Also note that the server uses this value to list the public links when no name was set; the new dialog shows "Public link")

I didn't realize it was still possible to create nameless links on the server. Then we must be consistent with the names displayed there. I didn't like the token-based name because it looked too technical, but believe consistency is more important.

the name field still applies only for creating new links (update the textbox on click and change the button label to "Update" instead of "Create"?)

The flow for creating/updating is hard to get right. If I did as you suggested and made the creation box do double duty as the name-edit box, one could not create new links unless there also was a way to deselect the current share: and it wouldn't be discoverable that deselecting was necessary to create more shares. I think the current way is comparatively clear. Adding a "Link properties" label might be helpful.

Also, updating the link name by directly clicking its name on the list does not perform the PUT request to update the name on the server.

Yes, there currently is no way of editing the name of a share. I'll look at it.

Actions for each link are stacked in a drop menu (open, copy, send link), when there's enough room for them in the list view

I used the menu because in my opinion it's easier to discover what the actions do this way, because there's obvious text. The button should probably get a "..." label to make it clearer more actions are hiding there. I have no strong opinion here.

The detailed permissions are still shown on the "Users and Groups" tab (ref. ckamm/owncloud-client@3d92c2c)

This patch was merged to 2.3 and hasn't yet been merged to master. It's just not included in this branch.

@ckamm
Copy link
Contributor Author

ckamm commented Apr 12, 2017

@SamuAlfageme Updated with some fixes!

@SamuAlfageme
Copy link
Contributor

@ckamm lookin' great 😎 I think it's good to be merged. Last comments:

The flow for creating/updating is hard to get right. [...] Adding a "Link properties" label might be helpful.

+1 to try how a label that separates somehow explicitly a "create" section from an "update" section; It might be also useful if possible to have the ability to deselect any entry on the table by clicking outside/on empty space.

I used the menu because in my opinion it's easier to discover what the actions do this way, because there's obvious text. The button should probably get a "..." label to make it clearer more actions are hiding there. I have no strong opinion here.

Agreed on the menu; however I think the most common use case for sharing links is to get copied, so a shortcut could also be nice.

And last difference with the web UI would be that there they have consecutive numbering on the name (by appending (n) to the link's default name) and we reuse the same name. I don't think this is necessary to be addressed at all.

As said, from an user's pov, this is good to go.

@guruz
Copy link
Contributor

guruz commented Apr 20, 2017

Code looks ok to me 👍

However

hudson.plugins.git.GitException: Command "git merge 8cb3a77022dfc8c688e3aeaf9fb08b6ddd305521" returned status code 1:
stdout: Auto-merging src/gui/shareusergroupwidget.cpp
Auto-merging src/gui/sharelinkwidget.ui
CONFLICT (content): Merge conflict in src/gui/sharelinkwidget.ui
Auto-merging src/gui/sharedialog.ui
CONFLICT (content): Merge conflict in src/gui/sharedialog.ui
Auto-merging src/gui/sharedialog.cpp
Automatic merge failed; fix conflicts and then commit the result.

ckamm added 4 commits April 21, 2017 10:09
There will probably be a ShareLinkLine too, due to owncloud#5655
Starting from oC 10.0.0 having several public link shares with
different attributes for a path will be supported. This adds
functionality to create, edit and delete these public link shares.

The behavior is currently gated by server version, but should be
adapted to use a capability as soon as one is introduced, see
owncloud/core#27622.

The UI reduces to a single-share version when talking to older servers.

Testing scenarios:
* Link sharing is disabled (by capability, not by theme)
* Required passwords
* Required expiry
* Forbidden 'allow upload' for folders
* New and old servers
* Allow creating nameless shares
* Display token as name for nameless shares
  (both to be consistent with server)
* Allow changing a share's name by editing it in the table
* Minor adjustments
@ckamm ckamm force-pushed the sharedialog-multiplelinkshares branch from b3e1e96 to 3c1a2cd Compare April 21, 2017 08:16
@ckamm
Copy link
Contributor Author

ckamm commented Apr 21, 2017

@guruz Rebased on top of master with some minor adjustments due to conflicting changes. I'll merge it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants