-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
docs(effects): tidy examples in effects lifecycle guide #2336
Conversation
Preview docs changes for c4e32f6 at https://previews.ngrx.io/pr2336-c4e32f6/ |
), | ||
{ dispatch: false }); | ||
|
||
ngrxOnRunEffects(resolvedEffects$: Observable<EffectNotification>) { |
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.
We want to use the HTML entities here, Observable<EffectNotification>
.
Otherwise the <
and >
brackets won't show up
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.
Ooh, I didn't realise that. Hmm I wonder if this can be improved somewhat as it makes maintenance really difficult. I suspect a better solution would be to make the CodeComponent
require a markdown preformatted block so that the source could be an actual typescript file. I'll fix this up, but raise a different issue to fix this properly.
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.
Hold off on creating an issue for now please, we (Brandon) is currently looking for alternatives for our docs.
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.
Oh right I already raised #2338 at least from that issue I will find out what the plan is ;)
eb9a0c4
to
e20a2b9
Compare
import { Observable, throwError } from 'rxjs'; | ||
import { retryWhen, mergeMap } from 'rxjs/operators'; | ||
import { ErrorHandler, NgModule } from '@angular/core'; |
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.
Could you add this import to the top of the imports (so that angular gets imported first)?
@@ -167,30 +166,29 @@ By default, effects are merged and subscribed to the store. Implement the `OnRun | |||
Usage: | |||
|
|||
<code-example header="user.effects.ts"> | |||
import { Observable } from 'rxjs'; | |||
import { exhaustMap, takeUntil, tap } from 'rxjs/operators'; | |||
import { Injectable } from '@angular/core'; |
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.
Same here
thanks for the fix @brandonroberts - sorry I was on holidays without access to github |
PR Checklist
Please check if your PR fulfills the following requirements:
- [ ] Tests for the changes have been added (for bug fixes / features)- [ ] Documentation has been added / updated (for bug fixes / features)PR Type
What kind of change does this PR introduce?
Does this PR introduce a breaking change?
Other information
It might be worth considering automating the embedding of code samples so this kind of thing is automatically caught in future.
One option I'd recommend Disclaimer I maintain this is embedme which allows for automatic insertion of source files into markdown documents. I'd be happy to raise a separate PR to partially demo the use of it within ngrx/platform