Add --welcome option to new and init so that it can be skipped with --no-welcome
#6868
Conversation
There was a problem hiding this comment.
Hi @romulomachado and thanks for your PR!
I have two comments about this PR:
-
Instead of adding a
skip-welcomeoption I'd propose awelcomeoption withdefault: true. That way--no-welcomewill automatically work and in.ember-cliyou could write"welcome": false. -
Instead of removing the code from the blueprint and later trying to add it again via
fscalls you should pass thewelcomeoption to thelocals()hook of the blueprint and use anifblock inside of theapplication.hbsto insert the welcome page component (see https://github.com/ember-cli/ember-cli/blob/master/blueprints/app/files/.travis.yml#L7 for an example)
There was a problem hiding this comment.
awesome! this looks pretty good already :)
could you also add tests for the new behavior to https://github.com/ember-cli/ember-cli/blob/master/tests/acceptance/new-test.js?
|
Hey @Turbo87, thanks for the suggestions! I tried sending the Thanks again for helping! I'm working on fixing the tests now. |
|
@Turbo87 ✅ Just added the tests! LMK if there's anything else I should do. |
|
Just realized it's not very friendly to have only the |
blueprints/addon/index.js
Outdated
| @@ -84,6 +84,8 @@ module.exports = { | |||
| let addonName = stringUtil.dasherize(addonRawName); | |||
| let addonNamespace = stringUtil.classify(addonRawName); | |||
|
|
|||
| let { yarn, welcome } = options; | |||
There was a problem hiding this comment.
I think destructuring is not yet supported on Node 4
lib/commands/new.js
Outdated
| { name: 'skip-npm', type: Boolean, default: false, aliases: ['sn'] }, | ||
| { name: 'skip-bower', type: Boolean, default: false, aliases: ['sb'] }, | ||
| { name: 'skip-git', type: Boolean, default: false, aliases: ['sg'] }, | ||
| { name: 'welcome', type: Boolean, default: true }, |
There was a problem hiding this comment.
since welcome by itself isn't that descriptive it would probably make sense to add a description here too
tests/acceptance/new-test.js
Outdated
| .to.match(/"ember-welcome-page"/); | ||
|
|
||
| expect(file('app/templates/application.hbs')) | ||
| .to.contain(/"{{welcome-page}}"/); |
There was a problem hiding this comment.
I think the issue is that you wrote /"{{welcome-page}}"/ instead of /{{welcome-page}}/. Could you try this without the quotes? or just use expect(...).to.contain('{{welcome-page}}'); instead.
There was a problem hiding this comment.
@Turbo87 That was it! Thank you, just updated it!
romulomachado
force-pushed
the
add-skip-welcome-page-option
branch
from
925c1f5
to
4ea6562
Compare
|
@Turbo87 Just rebased the branch (went from 18 commits to 5). |
romulomachado
changed the title
new and initnew and init so that it can be skipped with --no-welcome
|
there seems to be one failing test left, but I'm not sure why that is actually failing... 🤔 any ideas? |
|
☔ The latest upstream changes (presumably e5f3a33) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@Turbo87 Had a busy week last week. Hopefully, I'll have some time to fix that failing test this week. I have no idea why it's failing, but I'll investigate. |
romulomachado
force-pushed
the
add-skip-welcome-page-option
branch
2 times, most recently
from
38f04a9
to
08a8aac
Compare
|
☔ The latest upstream changes (presumably ef31f8f) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I'm late to the party, and this idea can be a separate PR, but it seems like if we had a good extendable commands story, the welcome addon could just extend the new command and add it's option, and a hook system would allow it to no-op on that option. |
|
@romulomachado could you rebase the PR on top of the |
Add welcome option to init command Add welcome option to new command
romulomachado
force-pushed
the
add-skip-welcome-page-option
branch
from
2360e25
to
06931e9
Compare
|
@Turbo87 ✅ Done! |
|
@homu r+ |
|
📌 Commit 06931e9 has been approved by |
|
⚡ Test exempted - status |
…wjblue Add --welcome option to `new` and `init` so that it can be skipped with --no-welcome Closes #6444
Closes #6444