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

feat(devtools): add 'copy' button in devtools #4468

Merged
merged 17 commits into from Nov 13, 2022

Conversation

StefanDjokovic
Copy link
Contributor

@StefanDjokovic StefanDjokovic commented Nov 6, 2022

This PR adds a copy button in the devtools. This will appear next to objects in Data Explorer.

Let me know if the design of this makes sense or how this can be improved.

Screenshot 2022-11-06 at 19 56 46

image

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 6, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 54cc881:

Sandbox Source
@tanstack/query-example-react-basic-typescript Configuration
@tanstack/query-example-solid-basic-typescript Configuration
@tanstack/query-example-vue-basic Configuration

@StefanDjokovic StefanDjokovic changed the title feat(devtools): add copy object data explorer feat(devtools): add 'copy' button in devtools Nov 6, 2022
Copy link
Collaborator

@TkDodo TkDodo left a comment

Choose a reason for hiding this comment

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

Nice work, thank you 🙏

I think some sort of user confirmation that the copying worked would be nice. Maybe the button could turn into a green checkmark or so for a short period of time before it turns back into the button via an animation?

FYI, in the codesandbox iframe, it doesn't seem to work, which got me confused. Opening it in full view does work, but the ux is the same: you click and nothing happens...

also, please add some tests for the new feature, thank you 🙏

packages/react-query-devtools/src/Explorer.tsx Outdated Show resolved Hide resolved
packages/react-query-devtools/src/Explorer.tsx Outdated Show resolved Hide resolved
packages/react-query-devtools/src/Explorer.tsx Outdated Show resolved Hide resolved
packages/react-query-devtools/src/Explorer.tsx Outdated Show resolved Hide resolved
@@ -124,6 +155,9 @@ export const DefaultRenderer: Renderer = ({
{subEntries.length} {subEntries.length > 1 ? `items` : `item`}
</Info>
</ExpandButton>
{copiable ? (<CopyButton>
<Copier onClick={() => navigator.clipboard.writeText(displayValue(value))} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

putting displayValue into the clipboard seems weird, because we use superjson to show non-serializable values. Why don't we just put the raw values that are really in the cache into the clipboard?

@StefanDjokovic StefanDjokovic marked this pull request as draft November 6, 2022 20:06
@StefanDjokovic
Copy link
Contributor Author

Thank you TkDodo for the feedback and thorough review 🙏

Will be adding the confirmation, tests, and investigate why it's not working in the iframe. Thanks for pointing them out! :)

@StefanDjokovic
Copy link
Contributor Author

StefanDjokovic commented Nov 6, 2022

Looks like it might not be possible to add to clipboard from the iframe :/

DOMException: The Clipboard API has been blocked because of a permissions policy applied to the current document. See https://goo.gl/EuHzyv for more details.

@StefanDjokovic
Copy link
Contributor Author

Confirmation state added (same SVG as GitHub):

image

I'm still unsure about how to best handle what is being pasted to the clipboard. I used superjson to stringify the value because I couldn't find any way to copy the javascript object :/ Let me know if you have some pointer on how to do that!

To handle situations where the user can't copy to clipboard (e.g. permissions in iframe) I've added an error state that should hopefully make things clearer.

image

The other solution that I have been thinking of is, instead of a copy-to-clipboard action, having it become a "print on console". That should help with the error state. Let me know your thoughts and any feedback/comments on the code :)

@StefanDjokovic StefanDjokovic marked this pull request as ready for review November 9, 2022 17:52
packages/react-query-devtools/src/Explorer.tsx Outdated Show resolved Hide resolved
packages/react-query-devtools/src/Explorer.tsx Outdated Show resolved Hide resolved
packages/react-query-devtools/src/Explorer.tsx Outdated Show resolved Hide resolved
packages/react-query-devtools/src/Explorer.tsx Outdated Show resolved Hide resolved
@StefanDjokovic
Copy link
Contributor Author

Thanks, fixed the comments 🙏

@codecov-commenter
Copy link

codecov-commenter commented Nov 12, 2022

Codecov Report

Base: 96.36% // Head: 92.25% // Decreases project coverage by -4.10% ⚠️

Coverage data is based on head (71f3950) compared to base (eab6e2c).
Patch has no changes to coverable lines.

❗ Current head 71f3950 differs from pull request most recent head 54cc881. Consider uploading reports for the commit 54cc881 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4468      +/-   ##
==========================================
- Coverage   96.36%   92.25%   -4.11%     
==========================================
  Files          45       89      +44     
  Lines        2281     3706    +1425     
  Branches      640      970     +330     
==========================================
+ Hits         2198     3419    +1221     
- Misses         80      271     +191     
- Partials        3       16      +13     
Impacted Files Coverage Δ
src/devtools/styledComponents.ts
src/devtools/tests/utils.tsx
src/react/Hydrate.tsx
src/core/subscribable.ts
src/core/focusManager.ts
...rc/createWebStoragePersistor-experimental/index.ts
src/react/useQueries.ts
src/react/QueryErrorResetBoundary.tsx
src/core/logger.ts
src/react/setLogger.ts
... and 124 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@StefanDjokovic
Copy link
Contributor Author

Formatted the test file, sorry about that, didn't realise that 😅

@TkDodo
Copy link
Collaborator

TkDodo commented Nov 12, 2022

awesome work, thank you 🚀

@TkDodo
Copy link
Collaborator

TkDodo commented Nov 12, 2022

I can see some act warnings in the react17 tests - can you take a look at them maybe?

@StefanDjokovic
Copy link
Contributor Author

@TkDodo I wasn't able to reproduce the warnings or last Fail/Warnings locally.

I tried running npm run test:react:17 and they don't show up, is there anything else that needs to be done to have them? I saw the warnings before when I wasn't importing act from @testing-library/react, but they seem to be working fine now locally

@TkDodo TkDodo merged commit d99d94e into TanStack:main Nov 13, 2022
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.

None yet

3 participants