Skip to content

adding copy url functionality to socialshare#417

Closed
ayezinzu wants to merge 9 commits intomasterfrom
copyurl-socialshare
Closed

adding copy url functionality to socialshare#417
ayezinzu wants to merge 9 commits intomasterfrom
copyurl-socialshare

Conversation

@ayezinzu
Copy link

@ayezinzu ayezinzu commented Jul 9, 2023

No description provided.

Copy link
Member

@alexiglesias93 alexiglesias93 left a comment

Choose a reason for hiding this comment

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

Please add tests!

Comment on lines 20 to 21
// if platform is copy button
if (platform === 'copy') {
Copy link
Member

Choose a reason for hiding this comment

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

If there was an award for the most redundant comments, this one would definitely be a candidate to win 😆

Copy link
Author

@ayezinzu ayezinzu Jul 20, 2023

Choose a reason for hiding this comment

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

Yeah, thanks for pointing this out, I figured what I was doing something wrong here 😅

Is it because other devs have done the same thing in the past?


// if platform is copy button
if (platform === 'copy') {
await triggerCopyShare();
Copy link
Member

Choose a reason for hiding this comment

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

Jokes aside, this should not be placed here.
You're triggering the copy before even checking if the trigger that was clicked was the copy.
Also, that await here is not needed. Although copying is async, you are not performing any other actions afterwards to it's unnecessary to await for it.

Copy link
Author

Choose a reason for hiding this comment

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

Updating in next commit!

try {
await navigator.clipboard.writeText(windowUrl);
} catch (err) {
Debug.alert('MESSAGE_TO_SHOW', 'error');
Copy link
Member

Choose a reason for hiding this comment

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

We're deprecating the Debug component in v2.
You can just use console.error.
Also, MESSAGE_TO_SHOW?

Copy link
Author

Choose a reason for hiding this comment

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

Updating in next commit!

Comment on lines 56 to 58
copy() {
// doesn't need to store any data and instances are irrelevant
return;
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it would make sense to create the specific callback for each platform inside the creators.
This way you will be able to treat the copy as any other platform.

Copy link
Author

Choose a reason for hiding this comment

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

I have updated the logic a bit to address this thing. Might be a different approach from what you proposed as I thought it adapted more to how this is built. Let me know your thoughts!

/**
* Defines a copy social button
*/
copy: generateDynamicAttibuteValue(COPY_ELEMENT_KEY),
Copy link
Member

Choose a reason for hiding this comment

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

copy doesn't require interacting with other elements so it's not necessary to be a dynamic attribute.

Copy link
Author

Choose a reason for hiding this comment

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

How would we get the selectors for the copy button then? @alexiglesias93

@robertkibet
Copy link
Contributor

@ayezinzu @alexiglesias93 this PR is supposed to use v1 or v2? 🤔

@ayezinzu
Copy link
Author

@ayezinzu @alexiglesias93 this PR is supposed to use v1 or v2? 🤔

I believe v1
@alexiglesias93 can confirm.

@ayezinzu
Copy link
Author

Hey team, I will come back with the updates for the feedback provided early this week!

@vercel
Copy link

vercel bot commented Jul 20, 2023

@IftodiRadu is attempting to deploy a commit to the Finsweet Team on Vercel.

To accomplish this, @IftodiRadu needs to request access to the Team.

Afterwards, an owner of the Team is required to accept their membership request.

If you're already a member of the respective Vercel Team, make sure that your Personal Vercel Account is connected to your GitHub account.

@vercel
Copy link

vercel bot commented Jul 20, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
attributes ❌ Failed (Inspect) Jul 20, 2023 3:36pm

@vercel
Copy link

vercel bot commented Aug 8, 2023

@ayezinzu is attempting to deploy a commit to the Finsweet Team on Vercel.

To accomplish this, @ayezinzu needs to request access to the Team.

Afterwards, an owner of the Team is required to accept their membership request.

If you're already a member of the respective Vercel Team, make sure that your Personal Vercel Account is connected to your GitHub account.

Copy link
Member

@alexiglesias93 alexiglesias93 left a comment

Choose a reason for hiding this comment

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

@ayezinzu CI is failing

@ayezinzu
Copy link
Author

ayezinzu commented Aug 8, 2023

@ayezinzu CI is failing

@alexiglesias93 should be fixed now

Copy link
Member

@alexiglesias93 alexiglesias93 left a comment

Choose a reason for hiding this comment

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

Also, please add a changeset

Comment on lines 28 to 30
const dataUrlAttribute = await copyLocator.getAttribute('data-url');

await expect(dataUrlAttribute).toEqual(currentUrl);
Copy link
Member

Choose a reason for hiding this comment

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

this is quite redundant, you should be checking if the clipboard contains the correct text:

  const clipboardText = await page.evaluate("navigator.clipboard.readText()");
  expect(clipboardText).toEqual(currentUrl);

Copy link
Author

Choose a reason for hiding this comment

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

@alexiglesias93 Firefox only supports reading the clipboard in browser extensions, using the "clipboardRead" extension permission. Thus, we can't access that property on Firefox. That is why we use clipboard.js which seems to be working fine.

Copy link
Member

@alexiglesias93 alexiglesias93 Aug 10, 2023

Choose a reason for hiding this comment

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

Then you could test by using execCommand to trigger a Paste action into a textarea, and check the contents that were pasted

Copy link
Author

Choose a reason for hiding this comment

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

Okay, will update to that.

});

// setting data-url attribute for testing purposes
copyButton.setAttribute('data-url', copyUrl);
Copy link
Member

Choose a reason for hiding this comment

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

this is not necessary, check my other comment for reference

Copy link
Author

Choose a reason for hiding this comment

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

Referenced in the comment here #417 (comment)

@ayezinzu
Copy link
Author

ayezinzu commented Aug 16, 2023

Also, please add a changeset

Hi @alexiglesias93
Have we not initiated any changesets on the master branch? I am unable to add a changeset

image

@vercel
Copy link

vercel bot commented Aug 15, 2024

An owner of the Finsweet Team on Vercel declined @ayezinzu's request to join.

In order for their commit to be deployed, @ayezinzu must push and request access again.

@alexiglesias93 alexiglesias93 deleted the copyurl-socialshare branch May 6, 2025 13:20
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.

4 participants