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

Add Copy Image option #79

Merged
merged 4 commits into from
Aug 30, 2019

Conversation

whitecrownclown
Copy link
Contributor

@sindresorhus

This PR follows up on sindresorhus/caprine#943.

A couple of mentions:

  • Electron can only copy png or jpg images. I tried with webp, for example, but electron.nativeImage has no support for it. I suppose we could convert to png or jpg.. thoughts?
  • I'm not sure if it should support file:// protocol or not
  • Used the request module for fetching the image buffer

Thanks!

@whitecrownclown whitecrownclown changed the title Add Save Image option [Feature] Add Save Image option Jun 25, 2019
@sindresorhus
Copy link
Owner

sindresorhus commented Jun 26, 2019

The implementation here is more complicated than I'd like. I think you instead should open an issue on https://github.com/electron/electron about adding an API to create a native image directly from an URL. They already have all the Chrome machinery to download images, so it would be better if they could handle it. Could be electron.nativeImage.createFromURL(url);. In your use-case and link to this PR. Maybe they can suggest even a better solution. Also mention the WebP problem.

readme.md Outdated Show resolved Hide resolved
@whitecrownclown whitecrownclown force-pushed the feat/save-image branch 7 times, most recently from 19186d0 to 75836cb Compare July 3, 2019 07:30
@whitecrownclown whitecrownclown changed the title [Feature] Add Save Image option [Feature] Add Copy Image option Jul 9, 2019
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

I think you could have spent more time on perfecting this PR. At this point, it would have been much faster for me to just implement it myself.

@sindresorhus sindresorhus changed the title [Feature] Add Copy Image option Add Copy Image option Aug 8, 2019
@whitecrownclown
Copy link
Contributor Author

Sorry about that, I’ll do better on future PRs.

@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus merged commit 79868f1 into sindresorhus:master Aug 30, 2019
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