-
Notifications
You must be signed in to change notification settings - Fork 24.8k
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: update text around changed obs examples #22920
Conversation
You can preview d6628b0 at https://pr22920-d6628b0.ngbuilds.io/. |
aio/content/guide/reactive-forms.md
Outdated
@@ -940,8 +940,7 @@ with that hero's data values. | |||
|
|||
A refresh button clears the hero list and the current selected hero before refetching the heroes. | |||
|
|||
Notice that `hero-list.component.ts` imports `Observable` and `finally` while `hero.service.ts` imports `Observable`, `of`, | |||
and `delay` from `rxjs`. | |||
Notice that `hero-list.component.ts` imports `Observable` and the `finally` operator, while `hero.service.ts` imports `Observable`, `of` and the `delay` operator from `rxjs`. |
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.
Minor: Google style uses Oxford comma. "...imports Observable, of, and the delay operator from rxjs."
aio/content/guide/reactive-forms.md
Outdated
@@ -940,8 +940,7 @@ with that hero's data values. | |||
|
|||
A refresh button clears the hero list and the current selected hero before refetching the heroes. | |||
|
|||
Notice that `hero-list.component.ts` imports `Observable` and `finally` while `hero.service.ts` imports `Observable`, `of`, | |||
and `delay` from `rxjs`. | |||
Notice that `hero-list.component.ts` imports `Observable` and the `finally` operator, while `hero.service.ts` imports `Observable`, `of` and the `delay` operator from `rxjs`. |
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.
Example: Please double-check. Looking at hero-list.component.ts, it looks like the import is "finalize" instead of "finally".
@IgorMinar @jasonaden I noticed that most changes are editorial. I just confirmed with @jbogarthyde. "The doc never described the code in enough detail to need alteration. The changes were mostly to how rxjs things are imported -- there was one very minor text change that resulted from that." (in reactive forms doc) |
d6628b0
to
03ace23
Compare
You can preview cdeeb1e at https://pr22920-cdeeb1e.ngbuilds.io/. |
You can preview 1fb1304 at https://pr22920-1fb1304.ngbuilds.io/. |
@jbogarthyde Thanks for fixing those legacy issues by changing HeroService in text to match the HeroesService we see in the examples. (unrelated to rxjs changes) @mhevery HTTP and Reactive Forms have content changes related to the examples and rxjs. Other files are just editorial to match style guide or how we refer to observables elsewhere. IMO this is ready to merge. Q: Do you want @jbogarthyde to merge the commits first? Thanks for stepping in! |
@mhevery @IgorMinar Hi. Can one of you approve, please...or let us know what we need to do/change before approval? Thanks! |
@IgorMinar While you were away, Brad and @mhevery said you need to be the one to review and approve this. Thanks. |
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.
looks good.
I'm a bit surprised that no bigger changes were necessary given all the changes that happened to the example code, but I'm not aware of any particular sections that still need an update.
caretaker note: I'm not sure why the circleCI jobs didn't fire on this PR, but the changes in this PR are docs only so it doesn't matter. If we do see other PRs with CI results missing we should investigate. |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
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: N/A
#22890
What is the new behavior?
Does this PR introduce a breaking change?
Other information
mostly cosmetic changes