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

Adding ‘Hello Electron’ to Boilerplate Section of Readme #399

Closed
wants to merge 1 commit into from

Conversation

erictherobot
Copy link

By submitting this pull request, I promise I have read the contributing guidelines twice and ensured my submission follows it. I realize not doing so wastes the maintainers' time that they could have spent making the world better. 🖖

  • I believe this should be included in the boilerplate section because
    it’s a helpful tool to kick off various projects using React +
    Electron. Or Tone.js + Electron… and others.
  • Also, I’ve read the Contribution Guidelines twice.

- I believe this should be included in the boilerplate section because
it’s a helpful tool to kick off various projects using React +
Electron. Or Tone.js + Electron… and others.
- Also, I’ve read the Contribution Guidelines twice.
@sindresorhus
Copy link
Owner

Some feedback:

  • You're not really showing off much of Electron in there. 95% of it would apply to a web app too. And the actual Electron code is mostly just the default Electron skeleton. Use some more Electron APIs.
  • Each branch readme needs a better explanation and docs of what it provides and its intent.
  • The items in the Branches section should be linkable, so you can easily click into one of the branches.
  • You're not doing users any favors by using electron-packager directly. I would strongly suggest using electron-builder instead, which requires less config and is much more comprehensive.
  • babel-preset-es2016-node5, Electron is based on Node.js 6, so I would recommend the https://github.com/salakar/babel-preset-node6 preset instead (Less transpilation!).
  • Why is this using a custom script? Seems like rm -rf would be enough.

@sindresorhus
Copy link
Owner

Closing for lack of response.

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