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

Public link share consistent ui #2841

Merged
merged 16 commits into from
Jan 16, 2020
Merged

Conversation

kulmann
Copy link
Member

@kulmann kulmann commented Jan 10, 2020

Description

Refactors the links sidebar into being more similar to / consistent with the collaborators sidebar. In detail:

  • links sidebar looks more similar to collaborators sidebar (no gray background anymore)
  • creating and editing public links is not an in-place form anymore but a separate panel which replaces the sidebar when active.
  • add a button for removing the expiration date of a public link (didn't exist before)
  • replace UI for picking roles for a public link with a dropdown, similar to collaborators (in fact reusing that exact vue component). was solved through radio buttons before.
  • change the add button for both public links and collaborators to being left aligned.
  • fix translation in collaborator role definitions

Related Issue

Motivation and Context

Consistency of UI/UX of sharing sidebar panels.

How Has This Been Tested?

  • Local testing on a Mac in chrome and firefox successful (features work as expected, refactoring didn't break things, adapted UI of public links is consistent with collaborators, having a full size creation/editing form in the sidebar and having roles being picked in a dropdown)
  • in drone selenium environment (iPhone test fails somehow)

Screenshots (if appropriate):

Bildschirmfoto 2020-01-10 um 16 04 42
Visual impression of the sidebar panel

Bildschirmfoto 2020-01-10 um 16 04 58
Visual impression of the public link form

Bildschirmfoto 2020-01-10 um 16 05 16
Dropdown for role selection active

Bildschirmfoto 2020-01-10 um 16 05 40
X for removing the expiration date of a public link

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)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@claassistantio
Copy link

claassistantio commented Jan 10, 2020

CLA assistant check
All committers have signed the CLA.

@kulmann kulmann requested review from PVince81 and removed request for DeepDiver1975 January 10, 2020 15:23
@PVince81 PVince81 requested a review from LukasHirt January 13, 2020 09:02
@kulmann kulmann force-pushed the public-link-share-consistent-ui branch from 87fa915 to 10b3162 Compare January 13, 2020 10:07
@kulmann kulmann force-pushed the public-link-share-consistent-ui branch from 10b3162 to b9bcc54 Compare January 13, 2020 20:29
@kulmann kulmann added Status:Needs-Review Needs review from a maintainer and removed Status:In-Progress labels Jan 13, 2020
@kulmann kulmann changed the title WIP: Public link share consistent ui Public link share consistent ui Jan 13, 2020
Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

Code and feature looks good, some minor comments

@PVince81
Copy link
Contributor

Regarding the iPhone test, I guess it's using a reduced window size so some UI elements might be rendered differently, like for example the file actions. Not sure why it would affect the public link panel though. AFAIK the right sidebar appears full screen in responsive mode.

@pmaier1
Copy link
Contributor

pmaier1 commented Jan 14, 2020

Thanks @kulmann, looks a lot better already.

Some feedback

  • Showing the public link token to the user is not so nice, IMO.
    • Actual requirement was to be able to see the link itself (not only the token)
    • I suggest to either remove the token completely (for now/MVP) or find an elegant way to show the whole link somewhere
  • Users frequently want to check how it will look like if a recipient uses the link: The link name could also be linked with the link itself.
  • Please change the explanatory note for the "Uploader" role to "Recipients can upload but existing contents are not revealed."
  • Explanation text for private links: "Only invited collaborators can use this link."
  • Explanation text for public links: "Anyone with the link can access this resource. No sign-in required."

  • I dislike the "Security settings" headline. Would maybe even drop it.
  • I dislike the "Link benennen:" and "Rolle:" headlines. Maybe you have an idea to make this self-explanatory.
  • The translation for the "Contributor" role ("Mitarbeiter") is bad as Collaborators would also translate to "Mitarbeiter". We should change it.

@PVince81
Copy link
Contributor

@kulmann regarding the test failure: please note that Selenium is trying to actually click on visible elements. Basically when waiting for an element to be visible and clicking on it, the element needs to actually appear in the viewport. For clicking, Selenium is not using JS click events but actually simulating a mouse click on a specific screen location.

With that knowledge, I suspect that in the iPhone screen size case the button for link creation doesn't fit on the screen so is not clickable by mouse. You could try doing a local test with the screen resolution "375x812".

@kulmann
Copy link
Member Author

kulmann commented Jan 15, 2020

@kulmann regarding the test failure: please note that Selenium is trying to actually click on visible elements. Basically when waiting for an element to be visible and clicking on it, the element needs to actually appear in the viewport. For clicking, Selenium is not using JS click events but actually simulating a mouse click on a specific screen location.

With that knowledge, I suspect that in the iPhone screen size case the button for link creation doesn't fit on the screen so is not clickable by mouse. You could try doing a local test with the screen resolution "375x812".

@PVince81
You're right, it's the screen resolution. I tested it locally in chrome @ iPhone X resolution before I asked you, where everything was fine. What I didn't have in mind was that the iPhone resolution test only limits the screen resolution, which still include the browser controls etc. So we don't have a viewport of 375x812, but a browser window of that size. While reaching the Create button could be solved by just scrolling down, you can see the main problem in the following screenshot. The role button is in the (vertical) middle of the screen, dropdown doesn't fit above or below the button entirely. As it is above the button in selenium (see screenshot), it leaves the top of the viewport and can't be scrolled to.

What's the way to go with this? Change implementation of the role dropdown for small screens to something other than a dropdown? Or force the dropdown to appear below the button and then scroll down before the waitForElementVisible? 🤔

Bildschirmfoto 2020-01-15 um 10 27 30

@PVince81
Copy link
Contributor

@kulmann as far as I remember, Selenium automatically does a "scroll into view" so that the DOM element becomes visible.

What can happen however is that some other DOM element is overlapping so clicking on the location would click on something else

@PVince81
Copy link
Contributor

please note that we likely have similar tests for the collaborators view, also with that dropdown. so you could compare to see if there are differences

@kulmann
Copy link
Member Author

kulmann commented Jan 15, 2020

please note that we likely have similar tests for the collaborators view, also with that dropdown. so you could compare to see if there are differences

The difference is, that there are only three roles in the collaborators role dropdown, whereas the public link role dropdown has four. Four exceed the viewport to the top, three doesn't, which is the only reason why this works for collaborators and does not work for public links.

@kulmann
Copy link
Member Author

kulmann commented Jan 15, 2020

please note that we likely have similar tests for the collaborators view, also with that dropdown. so you could compare to see if there are differences

Found a solution for this: uk-drop has an option to disable the auto-flip to the top. In that case everything works fine, no changes to ODS required. The auto-flip itself was not even the problem - only in combination with the z-index of the dropdown being behind some other elements. But imho the flip to the top is irritating anyway, so I just disabled the auto-flip.

@kulmann
Copy link
Member Author

kulmann commented Jan 15, 2020

@pmaier1
Thanks for the insights! 👍
I only implemented some of your requests, as the main purpose of this PR was NOT to reach a certain look and feel, but to have the look and feel of the two panels (collaborators and links) (close to) identical.
That being said, here are some status comments on your feedback:

  • Showing the public link token to the user is not so nice, IMO.

    • Actual requirement was to be able to see the link itself (not only the token)
    • I suggest to either remove the token completely (for now/MVP) or find an elegant way to show the whole link somewhere

Didn't know about the requirement. I'm pretty new to Phoenix so I don't have many insights on plans, yet. From a UX perspective I agree that showing the token to the user is useless. I removed it and set the link on the name of the link. Again, from a UX perspective this is what a user would expect anyway - link name being the actual link.

  • Please change the explanatory note for the "Uploader" role to "Recipients can upload but existing contents are not revealed."

Done

  • Explanation text for private links: "Only invited collaborators can use this link."
  • Explanation text for public links: "Anyone with the link can access this resource. No sign-in required."

How do you want to add these? Tooltip on a little question mark icon?

I dislike the "Security settings" headline. Would maybe even drop it.

Agreed. I removed it.

I dislike the "Link benennen:" and "Rolle:" headlines. Maybe you have an idea to make this self-explanatory.

This is self explanatory already. ;-) What I don't like, is that Link benennen includes an action, while all the other labels don't do that. I would vote for changing the link name label to Bezeichnung in German and title, name or identifier in English.

The translation for the "Contributor" role ("Mitarbeiter") is bad as Collaborators would also translate to "Mitarbeiter". We should change it.

Not a problem of the code but of the translation. Are the two english terms ok (Contributor and Collaborator)?

@PVince81
Copy link
Contributor

@kulmann for the remaining items from @pmaier1 please post them as checkboxes in #1307 which we'll address in future sprints.

optionally you can also already tick checklist items there as I believe your PR resolves a lot of them :-)

Copy link
Contributor

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍 for the additional changes

@kulmann kulmann force-pushed the public-link-share-consistent-ui branch from 9743884 to d0936c4 Compare January 16, 2020 08:45
@kulmann kulmann force-pushed the public-link-share-consistent-ui branch from d0936c4 to 6c7f0cb Compare January 16, 2020 10:15
@pmaier1
Copy link
Contributor

pmaier1 commented Jan 16, 2020

Thanks for the response and changes you already made @kulmann 👌

How do you want to add these? Tooltip on a little question mark icon?

Not sure about that point. Afaik tooltips should be avoided because they are not so nice for mobile users (I'm not the expert here). My point is that the private link concept is not well understood by a majority of the users which is why we should add some short explanation. Public links also have some criticality as they are accessible by anyone who has the link (or guesses it) unless there's a password set. Informing the user about this is a hard requirement. I the oC10 frontend the private link explanation is a tooltip, the public link explanation is just text in the panel.

This is self explanatory already. ;-) What I don't like, is that Link benennen includes an action, while all the other labels don't do that. I would vote for changing the link name label to Bezeichnung in German and title, name or identifier in English.

I agree. "Name" in English then, I guess. Maybe we don't need the headlines, though. Would leave that decision up to you.

Not a problem of the code but of the translation. Are the two english terms ok (Contributor and Collaborator)?

Yeah, agreed. The English terms came were my ideas, so I like them unless someone complains :) Let's see how we can fix this in the translation. Not a topic for here.

@PVince81 PVince81 merged commit 074191b into master Jan 16, 2020
@delete-merged-branch delete-merged-branch bot deleted the public-link-share-consistent-ui branch January 16, 2020 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status:Needs-Review Needs review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Role interaction not consistent between collaborators and link share
5 participants