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

Feature: #8134 - copy to clipboard #8136

Closed

Conversation

pascalwengerter
Copy link
Contributor

Description

Figured I'd treat myself to this quickwin before going to bed :D Caniuse approves: https://caniuse.com/?search=navigator.clipboard.writeText

Related Issue

Motivation and Context

I couldn't test the hypothesis of #8134 locally since Safari complained about my dev setup (Chrome/Firefox work fine), please review carefully.

How Has This Been Tested?

  • Locally

Types of changes

  • New feature (non-breaking change which adds functionality)

@pascalwengerter pascalwengerter force-pushed the feature/8134 branch 2 times, most recently from 56904df to 7dbe88c Compare December 20, 2022 00:44
@ownclouders
Copy link
Contributor

ownclouders commented Dec 20, 2022

Results for acceptance oC10 https://drone.owncloud.com/owncloud/web/30865/36/1

💥 The acceptance tests failed on retry. Please find the screenshots inside ...

webUISharingPublicBasic-publicLinkCreate_feature-L165.png

webUISharingPublicBasic-publicLinkCreate_feature-L165.png

💥 The oC10SharingPublic1 tests pipeline failed. The build has been cancelled.

@pascalwengerter
Copy link
Contributor Author

I tried running the acceptance tests according to the guide in the docs, but it wouldn't work on my M1 Mac (failed with "outdated browser")

Failing in https://drone.owncloud.com/owncloud/web/30836/36/19 in CI

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Nice! 👍 Can you look into the reason why acceptance tests are failing?

@ownclouders
Copy link
Contributor

Results for acceptance oCIS https://drone.owncloud.com/owncloud/web/30887/53/1
💥 The oCISWebdavLockProtection tests pipeline failed. The build has been cancelled.

@pascalwengerter
Copy link
Contributor Author

Nice! 👍 Can you look into the reason why acceptance tests are failing?

I will, as pointed out above I couldn't get them to run locally as it worked in the past? I'll re-check one of these days, maybe I messed up some docker thing with the M1 chip

@pascalwengerter
Copy link
Contributor Author

Needed to dig a bit deeper here - turns out using the native browser clipboard is only available when using localhost or a secure connection (and hence fails with a self-signed cert/running acceptance tests over HTTP). I searched around and used the workaround mentioned here - using the native API if available, and falling back to the 3rd party implementation that was already used before if it's not available

@dschmidt
Copy link
Member

If the lib doesn't abstract both approaches away for us, is it worth pulling it in at all? It's just a single function invocation anyway, maybe just inline it?

@sonarcloud
Copy link

sonarcloud bot commented Dec 26, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

70.0% 70.0% Coverage
0.0% 0.0% Duplication

@pascalwengerter
Copy link
Contributor Author

If the lib doesn't abstract both approaches away for us, is it worth pulling it in at all? It's just a single function invocation anyway, maybe just inline it?

What do you think of https://vueuse.org/core/useClipboard/ with the legacy flag? :)

@dschmidt
Copy link
Member

Perfect, obviously 💫

@pascalwengerter pascalwengerter force-pushed the feature/8134 branch 3 times, most recently from d367bd9 to 0bf64d8 Compare December 27, 2022 00:03
title: $gettext('EOS path copied'),
desc: $gettext('The EOS path has been copied to your clipboard.')
})
}
Copy link
Member

Choose a reason for hiding this comment

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

I know you just copied existing code, but do we really want to expose the EOS string/name to a user?

Should we revisit this, while you're at it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect, obviously 💫

I'm not quite sure if I get the isSupported right though - reading https://github.com/vueuse/vueuse/blob/70f55d4e68fd8f100fe844dd883c89b6d6594d03/packages/core/useClipboard/index.ts#L67 it seems like I could omit it since it is basically supported through the legacy option anyways?

Now it's "only" fixing unit tests, no opinion about the EOS translation thought...it's a CERN-internal thing anyways and practically no-one will be able to "casually spin up Reva/oCIS/oc-web on an EOS and then be confused about those strings" ;P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And from what I remember they literally send each other EOS paths when referencing resources

@pascalwengerter
Copy link
Contributor Author

Closed as superseded by #8173, thanks @JammingBen for taking over!

@pascalwengerter pascalwengerter deleted the feature/8134 branch January 2, 2023 15:29
@micbar micbar mentioned this pull request May 3, 2023
89 tasks
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.

[FR] Use Clipboard API instead of Document.execCommand for Clipboard Actions
4 participants