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

Prompt: Replaced 'read' with high-level 'prompt' library #213

Merged
merged 2 commits into from
Apr 23, 2013

Conversation

passy
Copy link
Member

@passy passy commented Apr 12, 2013

/ref #199

This changes the prompt API slightly and adds the following features:

  • grunt-init style coloring
  • default can be a function (in preperation of "magic defaults")
  • if default is a boolean, it will display a 'y/n' prompt with proper
    validation
  • validations occurs after a value is entered and not after the whole wizard
    completed

Watch it in action: http://ascii.io/a/2855

(I actually opened a new PR instead of continuing #198, but this has nothing to do anymore with filters, so I feel this is justified. :P)

function evaluatePrompts(prompt) {
if (_.isFunction(prompt.default)) {
prompt.default = prompt.default();
} else if (prompt.default === true || prompt.default === false) {
Copy link
Member

Choose a reason for hiding this comment

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

} else if (typeof prompt.default === 'boolean') { ?

@sindresorhus
Copy link
Member

The error message on invalid input is a bit awkwards since it repeats the question:

error: Invalid input for Would you like to include Twitter Bootstrap for Sass?

Should rather be:

error: Invalid input

Otherwise lgtm

@passy
Copy link
Member Author

passy commented Apr 16, 2013

Exported as single module now. Monkey-patching is probably our best option at the moment, unless someone has a better idea.

@kevva
Copy link
Member

kevva commented Apr 16, 2013

Ouch, the prompt lib makes use of the color grey which makes questions invisible on Solarized Dark.

@passy
Copy link
Member Author

passy commented Apr 16, 2013

Yeah, I used to use Solarized Dark myself, but switch to Solarized Light for most tasks, because bower uses the same invisible colors.

@kevva
Copy link
Member

kevva commented Apr 16, 2013

Yeah, but we probably need to address this in some way since Solarized is pretty widely used or else people will create issues about it.

Bower has fixed their colours. Mocha haven't though which is pretty painful.

@passy
Copy link
Member Author

passy commented Apr 16, 2013

Relevant discussion: altercation/solarized#220

@kevva
Copy link
Member

kevva commented Apr 16, 2013

There's severals issues opened about it on the Solarized repo but it seems like he won't fix it.

@passy
Copy link
Member Author

passy commented Apr 16, 2013

That's unfortunate, because I definitely consider this a bug. :/

@passy
Copy link
Member Author

passy commented Apr 18, 2013

I'd like to go one step further and disable colors globally if yo is started with that option. Since you can add custom 'themes' to the colors module, it should be possible to reset everything. I'll look into that.

@addyosmani
Copy link
Member

I like that suggestion @passy.

/ref yeoman#199

This changes the prompt API slightly and adds the following features:

    - `grunt-init` style coloring
    - `default` can be a function (in preperation of "magic defaults")
    - if `default` is a boolean, it will display a 'y/n' prompt with proper
      validation
    - validations occurs after a value is entered and not after the whole wizard
      completed
@passy
Copy link
Member Author

passy commented Apr 23, 2013

We decided to postpone the color option, because it's not directly related. It will definitely be tackled soon, though.

I rebased on top of master, fixed the merge conflict and did some manual testing with various generators where I couldn't find any problems. Does anyone know of a more elaborate use of the prompt system somewhere else that this could break?

sindresorhus added a commit that referenced this pull request Apr 23, 2013
Prompt: Replaced 'read' with high-level 'prompt' library
@sindresorhus sindresorhus merged commit 663ed3f into yeoman:master Apr 23, 2013
@sindresorhus
Copy link
Member

Great stuff @passy, here are some cakes. Don't eat them all at once.

🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰 🍰

@passy
Copy link
Member Author

passy commented Apr 23, 2013

Eat_All_The_Cake

@passy passy deleted the new-prompt branch April 23, 2013 22:01
@robwierzbowski
Copy link

Will the read properties in yo be replaced by prompt properties, i.e current yeoman docs message => prompt description, and warning => prompt message?

@kevva
Copy link
Member

kevva commented Apr 23, 2013

Sorry, should have commented earlier but this didn't work for me. Answers didn't get passed on.

@passy
Copy link
Member Author

passy commented Apr 23, 2013

Sorry, should have commented earlier but this didn't work for me. Answers didn't get passed on.

Could you elaborate? Where does that happen?

@kevva
Copy link
Member

kevva commented Apr 23, 2013

I just tested with the webapp generator. If I chose to use RequireJS the first time for example and then ran it again without RequireJS the file would be identical anyway. I'm not sure that the default values even worked.

@passy
Copy link
Member Author

passy commented Apr 23, 2013

@kevva Thanks, looking into it.

passy added a commit that referenced this pull request Apr 23, 2013
@passy
Copy link
Member Author

passy commented Apr 23, 2013

@kevva Could you check again? I misinterpreted how API is supposed to be used, should be backwards-compatible now.

@kevva
Copy link
Member

kevva commented Apr 23, 2013

@passy, cake for you, works now! I do think we should remove the color for the text in the question though, which is just a matter of a oneliner.

@passy
Copy link
Member Author

passy commented Apr 23, 2013

@kevva Feel free to remove it. We can re-add it once we found a solution how to disable colors globally.

@kevva
Copy link
Member

kevva commented Apr 24, 2013

Yay, I can see again without having to mark the text 👓

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.

5 participants