- Notifications
You must be signed in to change notification settings - Fork1k
fix(site): updateuseClipboard to work better with effect logic#20183
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| }; | ||
| } | ||
| functionrenderUseClipboard<TInputextendsUseClipboardInput>(inputs:TInput){ |
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 have no idea why I made this generic originally
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.
Amazing thank you! I tried it out on my PR as well and it works wonderfully.
| * will dispatch an error message to the GlobalSnackbar | ||
| */ | ||
| onError?:(errorMessage:string)=>void; | ||
| clearErrorOnSuccess?:boolean; |
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 was going to say, as a gut reaction, I feel like I would expect this to clear the error on success by default, but actually it looks like we never useerror anywhere? Maybe we could even drop storing and exposing the error for now?
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 can see that. I feel like we'lleventually need it, and the code is already set up to account for it in the tests, so I think merging it in now has the least friction. But I'll look into removing the error behavior in a bit
ParkreinerOct 6, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
For context, theclearErrorOnSuccess prop isn't "new", because it was already in the Registry codebase for a few months after I copied this file over there
156f985 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
No issue to track – spurred by#20129
Basically, the
useClipboardhook worked just fine up until now because we could always derive the clipboard text from within a render. There was never a case before where we would need to derive the text from outside React, and then sync it with both React and the browser. This PR updates the API to make that use case much more ergonomic and avoid risks of infinite rendersChanges made
useClipboardAPI to require passing the text in via thecopyToClipboardfunction, rather than requiring that the text gets specified in render logiccopyToClipboardfunction always stays stable across all React lifecycles