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
Merged

Improvements to welcome screen #418

merged 4 commits into from
May 8, 2016

Conversation

Diggsey
Copy link
Contributor

@Diggsey 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
Copy link
Contributor Author

Diggsey commented May 8, 2016

Here's what it looks like:
screenshot

@brson
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
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
Copy link
Contributor

brson commented May 8, 2016

Otherwise this lgtm. Very nice.

@brson
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
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
@Diggsey Diggsey deleted the term-md branch May 8, 2016 12:35
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.

2 participants