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

fix: #98 , and make copy more compatible #144

Merged
merged 2 commits into from
Sep 21, 2022
Merged

fix: #98 , and make copy more compatible #144

merged 2 commits into from
Sep 21, 2022

Conversation

cloydlau
Copy link
Contributor

No description provided.

@josdejong
Copy link
Owner

josdejong commented Sep 19, 2022

Thanks for working out this fix Cloyd 👍. It looks good at first sight. I'll do some thorough testing tomorrow.

@josdejong
Copy link
Owner

I've done some testing @cloydlau, and had a good look at the code. I like that the code is a self-contained function, very neat!

Two small feedbacks:

  1. Can you add a type declaration copyToClipBoard (text: string) ? (I'll see if I can configure TypeScript more strictly so it will yell about this itself)
  2. Can you run npm run format to fix some linting issues? (I'll configure a Github Action to run npm run lint so any issues will popup automatically)

And one question: do you know whether pasting works on non-secure http origins? If not, is there a similar solution possible you think?

@cloydlau
Copy link
Contributor Author

Sure, all done. And for HTTP, I've tested it before in this way:

image

@josdejong
Copy link
Owner

Thanks for the updates, looks good!

I just came across one more issue though I'm not sure if we should fix it: after copying using the "legacy" solution, the editor loses focus. What do you think, should we restore focus afterwards?

@cloydlau
Copy link
Contributor Author

Indeed! The "legacy" solution is for outdated browsers and unsecured websites, in which case we may only gurantee the core functions. So my personal view is to leave it be.

@josdejong
Copy link
Owner

Yes that makes sense. Thanks!

@josdejong josdejong merged commit d43a646 into josdejong:main Sep 21, 2022
@josdejong
Copy link
Owner

The fix has been published in v0.7.5

@cloydlau
Copy link
Contributor Author

cloydlau commented Sep 21, 2022

My pleasure. Actually I made this PR because someone raised a same issue in my repo json-editor-vue (an universal Vue wrapper for svelte-jsoneditor). Not the first time lol. If I might venture to ask that would the doc of svelte-jsoneditor take section like 'Related projects'? The doc indeed provides Vue example yet no two-way binding, either no example for Vue 2. So this 'Related projects' might be helpful for the Vue users of svelte-jsoneditor. It'll be my pleasure to make a PR.

@josdejong
Copy link
Owner

Ah, nice, thanks!

I've added a section linking to wrapper libraries in the readme, right now only containing you Vue library: https://github.com/josdejong/svelte-jsoneditor#wrapper-libraries-for-specific-frameworks

@cloydlau
Copy link
Contributor Author

Thanks! It really encouraged me.

@josdejong
Copy link
Owner

You're welcome! Thanks for making the library easier to use in Vue 😊 .

Have a nice weekend.

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.

2 participants