-
Notifications
You must be signed in to change notification settings - Fork 199
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
Implement clickable color swatches to copy color props to clipboard #476
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's a few nits in there, like removing console logs or commented out stuff. all in all this is super clean!
the color conversions are a bit noisy and there's good color libs, but i think it's totally fine for this to have it's own functions in place. IF you wanted, no pressure here, to pull in say colorjs to do the conversion work, it'd also be cool to add a couple other color functions to the options: oklch
and p3
(rounded 1 or 2 decimal places) as more modern syntax offerings.
nice tooltip solution 👍🏻
would have been fun to use [popover]
, but needs a little bit better browser support (it's like about to drop in FF), and I dont think this use case really necessitates a rewrite to use it (:top-layer
).
just a few little things and i think this is g2g. thanks a bunch, folks will love it!
elements.forEach((element) => { | ||
element.addEventListener('click', function (e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a good moment to leverage event bubbling, for 1 listener instead of 1 on each swatch!? down to refactor it a bit? shouldnt effect the rest of the code, mostly removes this loop and adds a click guard for clicks on other stuff in the parent. thoughts? 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be down to implement it. I'll brush up on event bubbling and how to implement it properly.
<form> | ||
<p>Copy format</p> | ||
<div class="options"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont think this needs a refactor, this is more of a comment:
I personally would have used a
all in all, what you have here is accessible to keyboard and screen reader, so no refactor needed 👍🏻
Co-authored-by: Adam Argyle <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closes #467