-
Notifications
You must be signed in to change notification settings - Fork 17
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
Convert app to Electron app #101
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ A review job has been created and sent to the PullRequest network.
Check the status or cancel PullRequest code review here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. The new dependencies appear to be safe of any major issues and are relatively well-maintained. Lots of cool stuff you can do with Electron.
I have a small suggestion to make the createWindow
method more flexible by accepting the width
and height
values.
Reviewed with ❤️ by PullRequest
|
||
let mainWindow; | ||
|
||
function createWindow() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should you consider passing width
and height
in the createWindow
method and set them with default value of 800 and 600 respectively. This way, you will be able to passing any width or height dynamically. If the user of this method doesn't pass anything in, the width will be set to the default values.
function createWindow(width = 800, height = 600) {
mainWindow = new BrowserWindow({ width, height });
...
}
[](#74364198-945d-4af6-8e4b-d90e97c0ad26 "PullRequest Meta [Do Not Modify]")
![Image of Kingsten B](https://static.pullrequest.com/storage/reviewer_profiles/OE2AQiMDf7oHbjsQjyAYmDHkbHVflhPQdUtLSZLX.jpg) **Kingsten B**
011a4ab
to
b156be7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Warning
PullRequest detected a force-push on this branch. This may have caused some information to be lost, and additional time may be required to complete review of the code. Read More
This PR sets up Electron so that the game can be ran as a desktop app. The MacOS build has been tested to work properly, I am waiting on feedback from other team members whether the Linux and Windows versions work properly. I will only complete the merge once those are verified.