-
Notifications
You must be signed in to change notification settings - Fork 23
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(stark-core): add support for deep state navigation for states from lazy loaded modules. Adapt Showcase to make DemoModule and NewsModule lazy loaded #814
Conversation
7410c28
to
4f731e3
Compare
StarkLoginPageComponent, | ||
StarkPreloadingPageComponent, | ||
StarkSessionExpiredPageComponent, | ||
StarkSessionLogoutPageComponent | ||
], | ||
exports: [StarkLoginPageComponent, StarkPreloadingPageComponent, StarkSessionExpiredPageComponent, StarkSessionLogoutPageComponent], |
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.
Has Prettier been runned on this file ? Could you reformat this ? 😊
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.
Yes, in fact it was prettier
the one that changed this line ;)
{ | ||
name: "home", | ||
url: "", | ||
views: { "@": { component: HomeComponent } }, |
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.
Why having the views: {"@": {}}
here ?
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.
Well, because now that we implemented lazy loaded modules, the states graph was not correct and any navigation to lazy loaded modules failed.
So, I had to adapt the graph: basically I added the AppComponent as the root state of the app, so that means that the <ui-view>
is no longer in the root Ui Router state but in a child route, therefore the views: { "@": { component: xxxxx } }
is needed to render the page in that ui-view
.
{ | ||
name: "demo.action-bar", | ||
url: "/action-bar", | ||
views: { "@": { component: DemoActionBarComponent } } |
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.
Can't we simply declare our states as before like this:
{
name: "demo.action-bar",
url: "/action-bar",
component: DemoActionBarComponent
},
? 😊
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.
Nop, we need the views: { "@": { component: HomeComponent } }
construct, see my previous comment.
export const NEWS_STATES: Ng2StateDeclaration[] = [ | ||
{ | ||
name: "news", | ||
url: "^/news", |
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.
Why declare url: "^/news"
here and in demo module, declare url: "^/dropdown"
?
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 don't think we have url: "^/dropdown"
in the demo module... we do have url: "/dropdown"
.
In any case, if you are wondering about the ^
, that is used to avoid double slash "//" in the URL after the domain (https://github.com/angular-ui/ui-router/wiki/URL-Routing#absolute-routes-). Otherwise, when we navigate to a demo page we will have a URL with double slashes (http://localhost:3000///demo/somePage") 😕
4f731e3
to
c81fe00
Compare
@christophercr could you please rebase your PR ? 😊 |
c81fe00
to
e4885c1
Compare
…om lazy loaded modules. Adapt Showcase to make DemoModule and NewsModule lazy loaded ISSUES CLOSED: #810
e4885c1
to
6589846
Compare
Rebased 😉 |
ISSUES CLOSED: #810
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #810
What is the new behavior?
StarkSessionModule
in "stark-core" where it belongs.starkAppInitStateName
andstarkAppExitStateName
is now in theStarkSessionModule
in "stark-core" where it belongs.targetRoute
resolve from thestarkAppInitStateName
now checks if the target state is part of a lazy module. In that case it lazy loads the module first and then search for the right target state.CommonModule
instead of theBrowserModule
andBrowserAnimationsModule
, since those modules should be loaded only once in the app.Does this PR introduce a breaking change?
Other information
Adapted the
DemoModule
andNewsModule
in the Showcase to make them lazy loaded modules.