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

Copy to clipboard with link instead of button #35527

Closed
wants to merge 1 commit into from

Conversation

Signed-off-by: Christopher Ng <chrng8@gmail.com>
@Pytal Pytal added this to the Nextcloud 26 milestone Dec 1, 2022
@Pytal Pytal self-assigned this Dec 1, 2022
@Pytal
Copy link
Member Author

Pytal commented Dec 1, 2022

/backport to stable25

Comment on lines +41 to +42
<NcActionLink :aria-label="t('files_sharing', 'Copy public link to clipboard')"
:href="shareLink"
Copy link
Member

Choose a reason for hiding this comment

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

We can also use role="button" if really necessary

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

@Pytal Generally speaking, please use the revert action in those cases. So it's clear that this is a revert of a previous commit.
#35533

@Pytal
Copy link
Member Author

Pytal commented Dec 1, 2022

@Pytal Generally speaking, please use the revert action in those cases. So it's clear that this is a revert of a previous commit. #35533

As described in the description this is an addendum to #35240 not a revert, as it keeps the material design icons

@skjnldsv
Copy link
Member

skjnldsv commented Dec 2, 2022

As described in the description this is an addendum to #35240 not a revert, as it keeps the material design icons

What about the tests ?

@Pytal
Copy link
Member Author

Pytal commented Dec 2, 2022

What about the tests ?

Since it's in one commit then reverting would also revert what we want to keep, would you suggest reverting the commit then re-adding the changes we want to keep?

@skjnldsv
Copy link
Member

skjnldsv commented Dec 2, 2022

Generally speaking, I think it's more clear, when someone is out of context, what really happened if we revert a breaking PR.
Then you can open another PR and add back the changes for the material design icon.
Which are actually not at all what the original PR was designed to change.

If I have to pinpoint something later on and move up the blame history, not being clear that this PR was a revert because of a regression, it's going to make it harder.

Anyway... Not going to debate that further 😁

@Pytal
Copy link
Member Author

Pytal commented Dec 2, 2022

Closing in favour of #35533 + followup PR with new material design icons

@Pytal Pytal closed this Dec 2, 2022
@Pytal Pytal deleted the enh/a11y-share-link-button branch February 24, 2023 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants