-
Notifications
You must be signed in to change notification settings - Fork 216
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
+ [feat] add the location strategy option #265
Conversation
The angular-cli@1.7.1 bug(angular/angular-cli#9726) make the test failed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your PR!
I have added a few comments, could you take a look at it?
As for the question vs option comment, I will update the contributor's guide to better explain the design choices regarding this as it wasn't explained anywhere.
@@ -54,7 +62,7 @@ import { AppRoutingModule } from './app-routing.module'; | |||
<% } else if (props.ui === 'bootstrap') { -%> | |||
NgbModule.forRoot(), | |||
<% } else if (props.ui === 'ionic') { -%> | |||
IonicModule.forRoot(AppComponent, { locationStrategy: 'path' }), | |||
IonicModule.forRoot(AppComponent, { locationStrategy: <%- props.location === 'hash' ? "'hash'": "'path'" %> }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hash is the default here, so no need to be told explicitely for this use case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've worried about if ionic team change the default. So I prefer to not rely on its default value.
@@ -69,6 +77,16 @@ import { AppRoutingModule } from './app-routing.module'; | |||
], | |||
declarations: [AppComponent], | |||
providers: [ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Path is the default here, so nothing to add for this use case.
Also this should only be added when not using ionic, as it performs its own override, see ^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Path is the default here, so nothing to add for this use case.
The same to above.
Also this should only be added when not using ionic, as it performs its own override,
En. You are right.
generators/app/prompts.js
Outdated
@@ -104,5 +104,21 @@ module.exports = [ | |||
message: 'Do you want lazy loading?', | |||
default: false, | |||
when: props => props.ui !== 'ionic' | |||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A --hash-strategy
or --location-strategy
option would be preferable here instead of an additional question as it's not a common use case, and using hash should be avoided whenever possible according to the Angular team.
We try to maintain the number of questions as low as possible and move other settings to options to keep the generation process as focused as possible. 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hash strategy is more suitable for the mobile application scenarios.
OK, agree absolutely. --location-strategy
add location strategy: