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

Add click to copy behavior #119

Merged
merged 34 commits into from
Sep 29, 2023
Merged

Add click to copy behavior #119

merged 34 commits into from
Sep 29, 2023

Conversation

jamesnw
Copy link
Contributor

@jamesnw jamesnw commented Aug 15, 2023

Description

Adds Click to Copy behavior on each output format and on the fg and bg colors.

I used navigator.clipboard.writeText, as it appears to have sufficient cross-browser support. Our content is all plain text, and our use case is driven directly by user interaction, so I don't think we need to complicate things.

Steps to test/reproduce

Verify that clicking the button copies the correct value.

@jamesnw jamesnw linked an issue Aug 15, 2023 that may be closed by this pull request
@jamesnw jamesnw requested a review from stacyk August 15, 2023 18:34
@netlify
Copy link

netlify bot commented Aug 15, 2023

Deploy Preview for oddcontrast ready!

Name Link
🔨 Latest commit 392d049
🔍 Latest deploy log https://app.netlify.com/sites/oddcontrast/deploys/6516e647a9ae510008832f7b
😎 Deploy Preview https://deploy-preview-119--oddcontrast.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

stacyk and others added 3 commits September 25, 2023 21:26
* main:
  Automated dependency upgrades
  Automated dependency upgrades
  Bump actions/checkout from 3 to 4
  Automated dependency upgrades
  only one version of vite at a time
  Automated dependency upgrades
  Pin back vitest
  Automated dependency upgrades
  pin back vitest for now
  Automated dependency upgrades
  review
  add linkedin and color js links
  Automated dependency upgrades
@jamesnw jamesnw marked this pull request as ready for review September 26, 2023 15:20
@jgerigmeyer jgerigmeyer linked an issue Sep 26, 2023 that may be closed by this pull request
@@ -8,6 +8,6 @@

$page: 60rem;
$sm-column-break: 30em;
$sm-page-break: 40em;
$sm-page-break: 42em;
Copy link
Member

Choose a reason for hiding this comment

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

had to move this up since we added the icon for longer formats like p3 to fit

Comment on lines +95 to +97
@container tool (min-width: 15rem) {
--tool-font-size: 5cqi;
}
Copy link
Member

Choose a reason for hiding this comment

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

cqi ok to use?? I only added this new in-between size since it is only between 15 and 21 rem and p3 can be really long.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like cqi has good support. Nice to see it in use.

Copy link
Member

Choose a reason for hiding this comment

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

If we're using container queries, the units should have the same support.

// Animation Patterns
// ------------------

@keyframes grow-in {
Copy link
Member

Choose a reason for hiding this comment

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

I am growing the copied icon in, but since it doesn't exist after the timer I can't animate it out. I think it would be weird to animate all the copy icons in on page load. It doesn't really bother me that these switch as it might bring a bit more attention to the action you just took.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the way this animation works looks good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@stacyk It would be possible to have it exist longer, if that would be helpful. Or maybe that's part of the story where the arrow morphs into the checkmark?

Copy link
Member

Choose a reason for hiding this comment

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

@jamesnw I don't think having it there longer would fix it as the issue unless we add a class to the clipboard icon so I can fade it in only once it has been toggled (since I wouldn't want all clipboards to fade in on page load). Is that possible? It doesn't really bother me as it currently is but I think it could be a tiny bit improved (maybe not "worth it" at this time though).

Copy link
Contributor Author

@jamesnw jamesnw Sep 27, 2023

Choose a reason for hiding this comment

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

@stacyk My sense is that there is a way to add/remove classes and DOM elements in a way that would allow us to do what we want here, but agree it's probably not worth it as part of this PR.

Copy link
Contributor

@dvdherron dvdherron left a comment

Choose a reason for hiding this comment

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

(And my approval was for both the layout-shift issue and the click-to-copy styles)

This looks good to me @stacyk. One thing I noticed that doesn't necessarily need to be addressed here is that the focus outlines are super faint. Especially on Firefox:

image

Do the buttons look like this for you when in focus?

// Animation Patterns
// ------------------

@keyframes grow-in {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the way this animation works looks good.

Comment on lines +95 to +97
@container tool (min-width: 15rem) {
--tool-font-size: 5cqi;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like cqi has good support. Nice to see it in use.

@stacyk
Copy link
Member

stacyk commented Sep 27, 2023

@dvdherron I reworked this a bit to add some specific focus styles for icon-only links and buttons so when you get a chance, take another look. Not sure if @mirisuzanne wants to review this as well. I switched to use focus-visible more often to give mouse users a bit of a break.

@dvdherron
Copy link
Contributor

I reworked this a bit to add some specific focus styles for icon-only links and buttons

These new focus styles for the icon-only links look great 👍🏽

Copy link
Member

@mirisuzanne mirisuzanne left a comment

Choose a reason for hiding this comment

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

It all works great for me. But the copy buttons are very prominent visually. Maybe they could get a bit lighter icon color or something?

src/lib/components/ratio/index.svelte Show resolved Hide resolved
Comment on lines +95 to +97
@container tool (min-width: 15rem) {
--tool-font-size: 5cqi;
}
Copy link
Member

Choose a reason for hiding this comment

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

If we're using container queries, the units should have the same support.

@stacyk stacyk self-requested a review September 29, 2023 14:37
@stacyk
Copy link
Member

stacyk commented Sep 29, 2023

It all works great for me. But the copy buttons are very prominent visually. Maybe they could get a bit lighter icon color or something?

@mirisuzanne I lightened all icon-only btw/links in the main and footer areas (so clipboards and social icon links) and also made the clipboards next to the input a tad smaller. I think this works better so thanks for that suggestion.

@mirisuzanne
Copy link
Member

@stacyk looks great!

@jgerigmeyer jgerigmeyer merged commit 446b065 into main Sep 29, 2023
6 checks passed
@jgerigmeyer jgerigmeyer deleted the 100-copy-colors-to-clipboard branch September 29, 2023 15:40
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.

layout shift on certain widths when sliding Copy colors to clipboard
5 participants