-
Notifications
You must be signed in to change notification settings - Fork 20
docs(readme): add parts on app state and dispatching actions #8
Conversation
Not totally sure about the need for adding `router:` to application state, but the one for dispatching actions is needed. Also, the description in original PR #5 has a typo in the `import`.
import {routerActions} from '@ngrx/store-store'; | ||
|
||
// ... | ||
store.dispatch(routerActions.go('/path', { query: 'string' )); |
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.
@brandonroberts Query params can still be an object, right? Or do they have to be a string?
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.
Not sure about that, took it from original PR comments. OT: routerActions
there seems wrong, it's still as it was before: import { go } from '@ngrx/router-store';
: will update now.
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.
Although, looking at code, yes: query
is an optional any
, so you're right
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.
Query params have to be an object. Will you add the other actions you can dispatch (show, back, forward)?
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 consider adding the RouterState
interface to be optional. Thoughts @MikeRyan52 ?
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.
Will you add the other actions you can dispatch (show, back, forward)?
Didn't think about making that exhaustive, but why not ...
About query params, shouldn't they strictly be of type NavigationExtras
? Or at least, should the documentation hint at those query params and state where they will end up (i.e., to router.navigate(... , *here*)
?
// ... | ||
store.dispatch(go('/path', { query: 'string' })); | ||
store.dispatch(replace('/path', { query: 'string' })); | ||
store.dispatch(search({ query: 'string' )); |
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.
This example is missing a closing brace
Thanks! |
Not totally sure about the need for adding
router:
to application state, but the one for dispatching actions is needed. Also, the description in original PR #5 has a typo in theimport
.