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

Inital support of electron target #333

Merged
merged 16 commits into from
Oct 29, 2018
Merged

Inital support of electron target #333

merged 16 commits into from
Oct 29, 2018

Conversation

YoannHub
Copy link

@YoannHub YoannHub commented Jun 9, 2018

As of this implementation only windows target has been tested.

Choose web app and electron when scaffolding a new application.

npm run electron:build: build app for electron
npm run electron:run: run app in electron
npm run electron:package:win: build windows executable app

@YoannHub YoannHub requested a review from sinedied June 9, 2018 21:40
Copy link
Member

@sinedied sinedied left a comment

Choose a reason for hiding this comment

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

Nice work, it just needs a few additions to support linux/osx to merge it :)

I can help test the linux/osx part and make icons if needed.

@@ -0,0 +1,60 @@
const electron = require('electron')
Copy link
Member

Choose a reason for hiding this comment

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

Could it be possible to use TS also here? (for a later version, JS is fine for a first release!)

Seems that can be done, as seen with https://github.com/maximegris/angular-electron, but it does not seems pretty :/

<% if (props.target.includes('electron')) { -%>
"electron:run": "electron .",
"electron:build": "npm run env -s && ng build --prod --base-href ./",
"electron:package:win" : "electron-packager . --overwrite --platform=win32 --arch=ia32 --out=out --icon=src/assets/icon.ico",
Copy link
Member

Choose a reason for hiding this comment

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

use a dist-electron folder? (+ add it to .gitignore)

Then it would be nice to add win64/linux/mac commands, along with a electron:package to run them all at once.
Also, with this you should add a question like with cordova to choose target platforms ie: win32, win64, linux, osx then the PR would be complete :)

@@ -53,6 +61,9 @@
"cordova-plugin-statusbar": "^2.4.1",
"cordova-plugin-whitelist": "^1.3.3",
<% } -%>
<% if (props.target.includes('electron')) { -%>
"electron": "^1.8.2",
Copy link
Member

Choose a reason for hiding this comment

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

Don't forget to upgrade to latest 2.x version before merging!

"electron-winstaller": "^2.2.0",
"electron": "^2.0.6",
"electron-packager": "^12.1.0",
"electron-winstaller": "^2.6.4",
Copy link
Member

Choose a reason for hiding this comment

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

Is "electron-winstaller": "^2.6.4" actually used? Or is it needed by electron packager maybe?

Copy link
Member

@sinedied sinedied left a comment

Choose a reason for hiding this comment

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

Just a question :)

@sinedied sinedied changed the base branch from master to release/6.0.x September 27, 2018 13:45
@sinedied
Copy link
Member

@YoannHub Besides the conflict, is it still WIP or can it be merged? IMHO, it's only missing the target platform choice (ie win / linux / mac) but that could be done in a separate PR 🙂

@YoannHub
Copy link
Author

@sinedied I think we can merge it as is, it's been tested on Windows and Mac target, I have not been able to test on the Linux version

@sinedied sinedied changed the title [WIP] Inital support of electron target Inital support of electron target Sep 27, 2018
@sinedied sinedied merged commit e6799a3 into release/6.0.x Oct 29, 2018
@sinedied sinedied deleted the feature/electron branch October 29, 2018 10:10
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.

3 participants