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

Customize what is going to be copied / Copy everything together #193

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

italodeandra
Copy link

As requested by @dbismut on #189 I'm creating this PR so we can benefit from the CI.

@vercel
Copy link

vercel bot commented Apr 5, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pmndrs/leva/84XQ6Pqgfth6sZZg2XjJVtW3aa7h
✅ Preview: https://leva-git-fork-italodeandra-master-pmndrs.vercel.app

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 5, 2021

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 7a63d43:

Sandbox Source
leva-minimal Configuration
leva-busy Configuration
leva-scroll Configuration
leva-advanced-panels Configuration
leva-ui Configuration
leva-theme Configuration
leva-transient Configuration
leva-plugin-plot Configuration
leva-plugin-bezier Configuration
leva-plugin-spring Configuration

Comment on lines 63 to +67
hideCopyButton?: boolean
/**
* Change what will be copied when clicking the global copy button
*/
copy?: (values: unknown) => string
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder whether it might sense to group those options similar to the titleBar options?

Copy link
Author

Choose a reason for hiding this comment

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

I thought of that, but hideCopyButton works for both Input and Title Bar copy button.
But perhaps I could move copy into titleBar? What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

Any thoughts on this? I was planning on finishing the changes for this PR today.

@changeset-bot
Copy link

changeset-bot bot commented Apr 6, 2021

⚠️ No Changeset found

Latest commit: 7a63d43

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@dbismut
Copy link
Collaborator

dbismut commented Apr 6, 2021

I've just updated from master since I've upgraded deps and I don't want to run a yarn install every time I'm switching branch 😇

@dbismut
Copy link
Collaborator

dbismut commented Apr 6, 2021

Hey, I think the PR is nice. API wise, I would try to simplify things a bit:

  • Remove hideCopyButton
  • Move copy into titleBar.copy

The one drawback is that you won't be able to have the global copy while hiding the label copy buttons, but I think that's minor (cc @gsimone @n1ru4l let me know what you think).

Regarding the default global copy function, it's a bit inconsistent with the default copy function, since the default one uses input keys, when the default copy function would use input paths (in leva these are two different things, as the path includes folders and subfolders dotted paths). Not a major deal but I thought it was worth mentioning.

In terms of aesthetics, I would reduce the size of title bar icons as they look pretty huge now (at one point we'll add undo / redo buttons so we'll need more space eventually).

@italodeandra
Copy link
Author

Hi @dbismut,

I'm open for making the changes but I think I'm not the best person for it. This PR was meant as a POC actually.

@italodeandra
Copy link
Author

@dbismut I was planning on finishing the changes for this PR today. But here are some question I had on your previous comment.

  • Remove hideCopyButton

Why remove the hideCopyButton?

  • Move copy into titleBar.copy

You mean make titleBar a new object and keep copy inside of it instead of having copy at root?

Regarding the default global copy function, it's a bit inconsistent with the default copy function, since the default one uses input keys, when the default copy function would use input paths (in leva these are two different things, as the path includes folders and subfolders dotted paths). Not a major deal but I thought it was worth mentioning.

Can you explain to me what is wrong so I can fix it? Because my knowledge of the codebase is very low. I don't even use path on my use-cases.

@dbismut
Copy link
Collaborator

dbismut commented May 1, 2021

@italodeandra Hey sorry for stalling this, I'm currently focused on react-use-gesture, I'll make sure to review this next week!

@RodrigoHamuy
Copy link
Contributor

Hey @italodeandra @dbismut , any updates with this? I would love to see this happening, and also add a button to paste those values so that you can easily share leva states.

Also happy to help with what's left for this ticket if needed.

@italodeandra
Copy link
Author

Hi @RodrigoHamuy,

I'm not using leva at the moment. You can continue with this PR if you want. Let me know if you need any help.

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.

4 participants