-
-
Notifications
You must be signed in to change notification settings - Fork 49
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
CopyButton enhancements #346
Conversation
🦋 Changeset detectedLatest commit: 5686823 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
83a0921
to
110617f
Compare
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.
Nice enhancements. Suggestion to simplify forwarding the on:click
event
showMessage = true; | ||
let copyValue = typeof value === 'function' ? value() : value; | ||
navigator.clipboard.writeText(copyValue); | ||
dispatch('click'); |
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.
Instead of using dispatch
maybe just add another on:click
to forward the event. It's a pattern I've used elsewhere (NavItem, ToggleButton)
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.
Yeah I go back and forth on that. Ended up doing it this way to be more explicit about the event firing after the clipboard has been copied, instead of relying on the undocumented behavior that the event handlers fire in the order they are listed in the template. (Not that it's likely to change at this point)
Also this is very slightly closer to how it will be in Svelte 5. :) Anyway I can change it if you still would like me to.
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.
Nope, makes sense to me to have the explicit ordering, especially to align with how things will be going forward with Svelte 5, which I believe becomes...
<script>
const { onclick } = $props;
</script>
<Button
onclick={() => {
// ...
onclick();
}}
/>
(or something like that. just wrote that in a github comment without checking too much).
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.
Thanks @dimfeld
value
to be a function that returns a stringclick
event when the button is clicked