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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

New copy to clipboard shortcut feature and footer icon #27

Merged

Conversation

DSGDSR
Copy link
Contributor

@DSGDSR DSGDSR commented Apr 27, 2022

Hi! I just created this PR adding a new shortcut for copying the selected node. I leave some images here... tried to follow svg structure you have in the project 馃槂

Captura de pantalla 2022-04-27 a las 14 35 50

Captura de pantalla 2022-04-27 a las 14 35 56

Few days using JSON Hero t handle big json files and feels quite comfortable... awesome app
Let me know how it looks, in case you want me to add/edit something let me know! Thanks

@ericallam
Copy link
Member

Hi @DSGDSR, thanks for the contribution! I'll be taking a look at this in more detail on Friday. Glad you like JSON Hero and thanks for being our first outside PR 馃挴

@ericallam
Copy link
Member

This is a great addition! Thanks for the PR. I do have a couple of things that I think will need to be done before this can be merged:

  • Choose a different keyboard shortcut. It's a little dangerous overriding the default copy shortcut as it breaks copying anything else on the page when selecting some text and hitting CMD/CTRL+C
  • Don't JSON.stringify strings to prevent the extraneous "" from being copied to the clipboard. You can do that by checking for selectedInfo?.name === "string"
  • Ideally this would also work in the Tree View but currently it only works in the Column View. Not a dealbreaker for getting this PR merged as we could add support for the Tree View later without much problem.

@DSGDSR
Copy link
Contributor Author

DSGDSR commented Apr 29, 2022

Thanks for reviewing my proposal! Totally agree with you mate, will work on it tomorrow 馃憤馃徏 including the support for Tree View mode.

@DSGDSR
Copy link
Contributor Author

DSGDSR commented May 2, 2022

Hi @ericallam, just updated the branch with the changes:

  • Using shift+c now as shortcut, also thought about using cmd+u, but this would work for Windows too.
  • Isolated component for this specific shortcut as it is used in the TreeView too, this way it was easily implemented instead of adding it to useVirtualTree hook as the rest of shortcuts.
  • Checking the selected node name, in case it is an string the value would be copied to the clipboard without using JSON.stringify
  • Modified the SVG accordingly:

Captura de pantalla 2022-05-02 a las 12 32 17

<rect x="16" width="14" height="14" rx="1.53846" fill="currentColor" />
<path
d="M5.64,10.22H8.25V7a.39.39,0,0,1,.38-.39H10l-3-3L4,6.65H5.26A.39.39,0,0,1,5.64,7v3.18Zm3,.78H5.26a.38.38,0,0,1-.39-.39V7.43H3.11a.39.39,0,0,1-.28-.66L6.72,2.86a.39.39,0,0,1,.55,0l3.88,3.91a.38.38,0,0,1-.27.66H9v3.18a.38.38,0,0,1-.39.39Z"
stroke="#0f172a" stroke-width="0.35px" fill="#0f172a"
Copy link
Member

Choose a reason for hiding this comment

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

This attribute should be strokeWidth, not stroke-width

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment resolved 馃樅, thanks for the support and for accepting my proposal. Will be back if any feature comes to my mind while using jsonhero!

@ericallam
Copy link
Member

This PR will be ready to merge once the above comment is resolved. Great work, such an awesome feature 馃憤

@ericallam ericallam merged commit 3b0a5b8 into triggerdotdev:main May 3, 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

2 participants