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 window.loadFile and webContents.loadFile helper methods #11565

Merged
merged 2 commits into from Jan 22, 2018

Conversation

MarshallOfSound
Copy link
Member

Closes #11560

@MarshallOfSound MarshallOfSound requested review from a team January 3, 2018 22:41
@sindresorhus
Copy link
Contributor

What do you think about the idea of adding a file option to BrowserWindow as a convenience? Most users will want to load the local file right away, so would be nice to be able to specify it there instead.

loadURL() is not a BrowserWindow option as it has support for its own options, but loadFile() only accepts a string, so it could easily be an option in BrowserWindow.

@sindresorhus
Copy link
Contributor

@MarshallOfSound
Copy link
Member Author

What do you think about the idea of adding a file option to BrowserWindow as a convenience?

Not a fan tbh, I much prefer the explicit "create then navigate" approach we currently use. This method just makes it easier/safer for people to navigate to local files 👍

@MarshallOfSound
Copy link
Member Author

Maybe also update the docs here:

Yeah once this goes through I'll follow up with a PR to update docs and usage in our own code. I didn't realize we had so much string templating loadURL around the place 🤔

@felixrieseberg
Copy link
Member

Not a fan tbh, I much prefer the explicit "create then navigate" approach we currently use. This method just makes it easier/safer for people to navigate to local files 👍

I'm with you on that, especially because you can't attach listeners to something that hasn't been created yet - and therefore wouldn't be able to catch errors emitted via EventEmitter.

Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

I see a few things pending for future PRs in comments, but for the scope of this PR this looks good to me!

@MarshallOfSound MarshallOfSound merged commit 32a1395 into master Jan 22, 2018
@MarshallOfSound MarshallOfSound deleted the add-load-file-helper branch January 22, 2018 22:09
mifi added a commit to mifi/electron that referenced this pull request May 20, 2018
codebytere pushed a commit that referenced this pull request May 25, 2018
* Simplify loading of html

See new api: #11565

* Update first-app.md

* Update first-app.md
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

4 participants