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

updates to default app and cli usage #10633

Merged
merged 15 commits into from Oct 7, 2017

Conversation

Projects
None yet
6 participants
@codebytere
Copy link
Member

codebytere commented Sep 27, 2017

Tracks progress on #5345

Tasks:

  • Make help response the response for no args specified
  • Make default app a flagged option
  • show Chrome, Node, and V8 versions
  • link to electron-forge
  • remove references to prebuilt
  • link the logo to the website
  • Add more information to default-app, maybe some links to resources and examples?

Please feel free to add input!

Thought it could use some design sprucing:

screen shot 2017-09-27 at 11 56 18 pm

(I'm aware of some of the spacing/padding issues and am working on them)

/cc @zeke

codebytere added some commits Sep 27, 2017

@codebytere codebytere requested a review from as a code owner Sep 28, 2017

@codebytere codebytere self-assigned this Sep 28, 2017

</a>
</div>
<div class="linkcol">
<h4>Electron-Forge</h4>

This comment has been minimized.

Copy link
@malept

malept Sep 29, 2017

Member

I'd prefer either "Electron Forge" or "electron-forge", not the hybrid 😁

<div class="linkcol">
<h4>Electron-Forge</h4>
<a class="hero-link" href="https://electronforge.io">
<span class="octicon hero-octicon octicon-telescope" style="padding-left:2px" aria-hidden="true"></span>

This comment has been minimized.

Copy link
@malept

malept Sep 29, 2017

Member

Too bad there isn't an anvil octicon 😄

This comment has been minimized.

Copy link
@codebytere

codebytere Sep 29, 2017

Author Member

if you'd prefer a different one, let me know!

This comment has been minimized.

Copy link
@malept

malept Sep 29, 2017

Member

@MarshallOfSound will have an opinion on this

@zeke

This comment has been minimized.

Copy link
Member

zeke commented Sep 29, 2017

I took a shot at improving the styles, plus a few other changes:

  • Changed Chrome to Chromium
  • Made link text clickable
  • Added an unselectable $ prompt to make it more obvious that it's a command
  • Fix link handler, which was broken by links being inside <span> tags
  • Add utm_source query param to outbound links so we can track default_app link clicks.

Here's what it's looking like now:

screen shot 2017-09-29 at 12 13 02 pm

const URL = require('url')
const electronPath = path.relative(process.cwd(), remote.process.execPath)

Array.from(document.querySelectorAll('a[href]')).forEach(link => {

This comment has been minimized.

Copy link
@felixrieseberg

felixrieseberg Oct 6, 2017

Member

I got a minor preference for [...document.querySelectorAll('a[href]')], but it's the same thing, so whatever you feel like!

document.querySelector('.chrome-version').innerText = `Chromium v${process.versions.chrome}`
document.querySelector('.node-version').innerText = `Node v${process.versions.node}`
document.querySelector('.v8-version').innerText = `v8 v${process.versions.v8}`

This comment has been minimized.

Copy link
@felixrieseberg

felixrieseberg Oct 6, 2017

Member

Why this newline?

This comment has been minimized.

Copy link
@codebytere

codebytere Oct 6, 2017

Author Member

...unclear....
will change, whoops

@felixrieseberg
Copy link
Member

felixrieseberg left a comment

Took a peek at the code – looks good to me!

@codebytere codebytere changed the title [WIP] updates to default app and cli usage updates to default app and cli usage Oct 6, 2017

@vanessayuenn
Copy link
Contributor

vanessayuenn left a comment

Code looks good to me 👍
Is the outstanding item to only show --interactive tip on macOS and Linux still a to-do or won't be implemented?

@codebytere

This comment has been minimized.

Copy link
Member Author

codebytere commented Oct 6, 2017

won't be implemented for now :D

@codebytere codebytere merged commit 85ef1ee into master Oct 7, 2017

8 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/branch AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-mas-x64 Build #5339 succeeded in 12 min
Details
electron-osx-x64 Build #5327 succeeded in 13 min
Details

@codebytere codebytere deleted the update-default-app branch Oct 7, 2017

@zeke

This comment has been minimized.

Copy link
Member

zeke commented Oct 7, 2017

Glad to see this shipping. Curious about the deprecation notice, though:

Deprecation Warning: To render the default app, the -d or --default flags will soon need to be used.

By the time a user is reading this, doesn't the -d flag need to be used?

@codebytere

This comment has been minimized.

Copy link
Member Author

codebytere commented Oct 7, 2017

hmm, maybe i can word this more clearly? I changed it (following a chat with @felixrieseberg) such that the default app still renders with no args, as well as with new -d and --default flags, and told users that the default rendering would soon disappear and that the only way to see the default app would be with the new flags so that there was a transition into the breaking change. Is there a better warning you think might convey that line of thinking?

@MarshallOfSound

This comment has been minimized.

Copy link
Member

MarshallOfSound commented Oct 7, 2017

Deprecation Warning: To render the default app, the -d or --default flags will soon need to be used.

I actually didn't see this, with this change it means users simply launching "Electron.app" will not see anything, I think we should always show the default app like we currently do. Is there a reason / discussion about that change in behavior?

@MarshallOfSound

This comment has been minimized.

Copy link
Member

MarshallOfSound commented Oct 7, 2017

so that there was a transition into the breaking change.

FWIW just building current master and launching the output from finder results in the behavior I described above, the app icon appears briefly and then the app closes itself

@codebytere

This comment has been minimized.

Copy link
Member Author

codebytere commented Oct 7, 2017

hmm, that wasn't happening when i was testing locally 🤔 i discussed it with @zeke originally, just because i felt that the help information should be more of a 1st class citizen and not only available via --help flag. At the time it seemed like the best move was to move over default app and replace it with help text, but i dont see any reason we can't keep both, now that i think about it. what are y'alls thoughts?

@codebytere

This comment has been minimized.

Copy link
Member Author

codebytere commented Oct 7, 2017

Pushed that change to a branch: https://github.com/electron/electron/tree/remove-default-deprecation

Fixed the app close, an exit snuck in there :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.