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

fix(copy-button): use new clipboard functions #1734

Merged
merged 2 commits into from
Apr 27, 2023
Merged

Conversation

ju-Skinner
Copy link
Collaborator

@ju-Skinner ju-Skinner commented Apr 26, 2023

we still have a fallback to use execCommand for legacy browser support

JIRA Ticket: DSS-388

Description

It was discovered that the copy-button was not working in certain browsers (versions). The code implemented here addresses both current versions and legacy version support which falls back to using execCommand('copy').

Testing in sage-lib

  1. Navigate to the Copy Button component on the documentation site.
  2. In each browser, Chrome (v111 and v112), Safari, and Firefox confirm that the functionality behaves as expected.

Testing in kajabi-products

  1. (HIGH) Copy button is broken in Chrome version 112 or higher. Please check with all major browsers
    • Navigate here, under Account Details, scroll to API Credentials under and confirm the copy-button
    • Navigate here and click "Set up authenticator". Once you enter your password, you should see a copy button under the QR code. Confirm that it functions as expected.

we still have a fallback to use execCommand for legacy browser support

fix: DSS-388
@ju-Skinner ju-Skinner requested a review from a team April 26, 2023 20:15
@ju-Skinner ju-Skinner self-assigned this Apr 26, 2023
Copy link
Member

@pixelflips pixelflips left a comment

Choose a reason for hiding this comment

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

Seems to be working properly in Blink/Chromium browsers, but getting an error message in the toast for Webkit(Safari) and Gecko(FF).

Chrome 111 - Works
Chrome Beta 113 - Works
Chrome Canary 114 - Works
Edge 112 - Works
Firefox 112 - Error Message
Safari 16.4 - Error Message
Safari Tech Preview 168 - Error Message

Screenshot 2023-04-26 at 1 45 33 PM

@pixelflips pixelflips requested a review from a team April 26, 2023 21:14
@ju-Skinner
Copy link
Collaborator Author

ju-Skinner commented Apr 27, 2023

Seems to be working properly in Blink/Chromium browsers, but getting an error message in the toast for Webkit(Safari) and Gecko(FF).

Chrome 111 - Works Chrome Beta 113 - Works Chrome Canary 114 - Works Edge 112 - Works Firefox 112 - Error Message Safari 16.4 - Error Message Safari Tech Preview 168 - Error Message

Screenshot 2023-04-26 at 1 45 33 PM

@pixelflips Can you provide a bit more details about the Safari browser you tested with, please? I tested with my local version of Safari (16.4.1) and it works as expected. See the loom video for reference.

I will download Firefox and see what's going on there.

EDIT:

I was able to reproduce both failures in Safari and Firefox. It occurs when you use localhost:4000 as the server address. If you notice in my loom video I am using sage-design-system.kajabi.test/ which is a developer trick leveraging puma dev to forward to localhost and specific port (4000 in this case).

Let me dig in more as to why this fails when using localhost

This was done due to the vast differences between descriptors allowed between browsers

see https://developer.mozilla.org/en-US/docs/Web/API/Permissions/query for a list
@ju-Skinner ju-Skinner requested a review from pixelflips April 27, 2023 15:05
Copy link
Member

@pixelflips pixelflips left a comment

Choose a reason for hiding this comment

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

🔥 Rechecked all the same browsers as before and all is working as expected now. Great work!

@pixelflips pixelflips requested a review from a team April 27, 2023 15:43
Copy link
Member

@teenwolfblitzer teenwolfblitzer left a comment

Choose a reason for hiding this comment

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

👍 LGTM

Not related to this work: as a follow-up we may want the examples to differ in their text. Having the same repeated string makes it tricky to tell which button you've used.

@ju-Skinner ju-Skinner merged commit c65e356 into develop Apr 27, 2023
@ju-Skinner ju-Skinner mentioned this pull request Apr 27, 2023
@ju-Skinner ju-Skinner deleted the fix/copy-button branch October 24, 2023 15:07
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.

4 participants