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

Improvements to welcome screen #418

Merged
merged 4 commits into from May 8, 2016

Conversation

Projects
None yet
2 participants
@Diggsey
Copy link
Contributor

Diggsey commented May 8, 2016

  • Removes some code duplication from term2
  • Uses markdown to parse markdown-style text and display formatted output in the terminal
  • Implements a word-wrap algorithm
  • Shows install options before asking whether to customize
  • Uses numbered choices rather than letters

The markdown crate exposes a tokenize method which seems to be exactly fit for purpose. However, it exposes a private type in the return value which makes it unusable. I've opened an issue about it, but for the moment I've had to fork it and use a git dependency.

r? @brson

@Diggsey

This comment has been minimized.

Copy link
Contributor Author

Diggsey commented May 8, 2016

Here's what it looks like:
screenshot

@brson

This comment has been minimized.

Copy link
Contributor

brson commented May 8, 2016

This looks really cool. I love the way this emphasizes key info.

Here are the things I noticed from just running it:

  • On Unix, the path to .profile in the pre-install message should be indented two spaces
  • The red in WARNING looks dark here. Is there a bright variant?
  • The post install "Rust is installed now" message contains a leading "#". Not sure if that's intentional, but it looks out of place because I haven't seen another one.
  • The post install message might also bold "run source $HOME/.cargo/env". Maybe also bold PATH the same way as the pre-install message.

I'll give the code a review seperately.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented May 8, 2016

It looks like the markdown parsing is not applied to the post-install and pre-uninstall messages.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented May 8, 2016

Otherwise this lgtm. Very nice.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented May 8, 2016

r=me with the markdown processing applied to all the messages and with the bots passing.

This will break tests in cli-install-interactive for sure.

Diggsey added some commits May 8, 2016

@Diggsey

This comment has been minimized.

Copy link
Contributor Author

Diggsey commented May 8, 2016

That should be everything (just waiting on tests)

@Diggsey Diggsey merged commit 0de142a into master May 8, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Diggsey Diggsey deleted the term-md branch May 8, 2016

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.