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

Projects
None yet
4 participants
@MarshallOfSound
Member

MarshallOfSound commented Jan 3, 2018

Closes #11560

@MarshallOfSound MarshallOfSound requested review from electron/docs as code owners Jan 3, 2018

@sindresorhus

This comment has been minimized.

Contributor

sindresorhus commented Jan 4, 2018

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.

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Jan 4, 2018

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

This comment has been minimized.

Member

MarshallOfSound commented Jan 4, 2018

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

This comment has been minimized.

Member

felixrieseberg commented Jan 5, 2018

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.

@codebytere

codebytere approved these changes Jan 22, 2018 edited

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

6 checks passed

ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@MarshallOfSound MarshallOfSound deleted the add-load-file-helper branch Jan 22, 2018

mifi added a commit to mifi/electron that referenced this pull request May 20, 2018

codebytere added a commit that referenced this pull request May 25, 2018

docs: Simplify loading of html in example (#13013)
* 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