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.

Sign up for GitHub

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

Jump to bottom

Copy to clipboard with button instead of link #35240

Merged
merged 1 commit into from
Nov 26, 2022
Merged

Copy to clipboard with button instead of link #35240

merged 1 commit into from
Nov 26, 2022

Conversation

Pytal
Copy link
Member

@Pytal Pytal commented Nov 18, 2022

Before After
image image

@Pytal Pytal added this to the Nextcloud 26 milestone Nov 18, 2022
@Pytal Pytal self-assigned this Nov 18, 2022
@Pytal
Copy link
Member Author

Pytal commented Nov 18, 2022

/backport to stable25

@Pytal Pytal mentioned this pull request Nov 18, 2022
Closed
10 tasks
PVince81
PVince81 approved these changes Nov 18, 2022
View reviewed changes
Copy link
Member

@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.

👍

@Pytal
Copy link
Member Author

Pytal commented Nov 19, 2022

/rebase

@Pytal
Copy link
Member Author

Pytal commented Nov 21, 2022 •
edited
Loading

  • Fix legacy js failures

@Pytal Pytal added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Nov 23, 2022
@Pytal
Copy to clipboard with button instead of link
4ffc022 
Signed-off-by: Christopher Ng <chrng8@gmail.com>
@Pytal Pytal merged commit 1cebd4a into master Nov 26, 2022
@Pytal Pytal deleted the enh/a11y-share-button branch November 26, 2022 00:24
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Nov 26, 2022
Closed
1 task
@skjnldsv
Copy link
Member

skjnldsv commented Nov 26, 2022 •
edited
Loading

Noooooooo
There was multiple good reasons :)

  • If the copy fails for browser security reason, you cannot get the link.
  • The fact that it was a link also allowed you to right/middle click and copy or open in a new tab.
  • It also make screen readers and accessibility tools understand the data.

This is not a simple button, the automated copy link is secondary. That is why most UI shows an input AND a button to copy the content. We don't have enough space for that, so a link is the least we can do :)

It should be reverted imho

@skjnldsv
Copy link
Member

On a subsidiary note, it also prevent us from doing testing and retrieving the link.
Copy link is very unreliable on automated e2e testing

Viewer is failing because of this PR, that's how I found it

@Pytal
Copy link
Member Author

Pytal commented Nov 29, 2022

Good points @skjnldsv

Will confirm with the a11y reviewer as this was a specific request made during the audit which we would fail if not done

This was referenced Nov 30, 2022
Closed
Closed
@skjnldsv skjnldsv restored the enh/a11y-share-button branch December 1, 2022 08:21
@skjnldsv skjnldsv deleted the enh/a11y-share-button branch December 1, 2022 08:22
@skjnldsv skjnldsv mentioned this pull request Dec 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PVince81 PVince81 PVince81 approved these changes

@GretaD GretaD GretaD approved these changes

@JuliaKirschenheuter JuliaKirschenheuter Awaiting requested review from JuliaKirschenheuter

@julien-nc julien-nc Awaiting requested review from julien-nc

Assignees

@Pytal Pytal

Labels
4. to release Ready to be released and/or waiting for tests to finish accessibility feature: sharing
Projects
None yet
Milestone
Nextcloud 26
Development

Successfully merging this pull request may close these issues.
4 participants
@Pytal @skjnldsv @PVince81 @GretaD