Skip to content
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

Closed

Conversation

jbogarthyde
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[x] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A
#22890

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

mostly cosmetic changes

@mary-poppins
Copy link

You can preview d6628b0 at https://pr22920-d6628b0.ngbuilds.io/.

@jenniferfell jenniferfell added the target: major This PR is targeted for the next major release label Mar 23, 2018
@jenniferfell jenniferfell added this to the v6.0 milestone Mar 23, 2018
@@ -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`.
Copy link
Contributor

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."

@@ -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`.
Copy link
Contributor

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".

@jenniferfell
Copy link
Contributor

@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)

@mary-poppins
Copy link

You can preview cdeeb1e at https://pr22920-cdeeb1e.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 1fb1304 at https://pr22920-1fb1304.ngbuilds.io/.

@jenniferfell jenniferfell modified the milestones: v6.0, v6-candidates Mar 23, 2018
@jenniferfell
Copy link
Contributor

@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!

@jenniferfell
Copy link
Contributor

@mhevery @IgorMinar Hi. Can one of you approve, please...or let us know what we need to do/change before approval? Thanks!

@jenniferfell
Copy link
Contributor

@IgorMinar While you were away, Brad and @mhevery said you need to be the one to review and approve this. Thanks.

Copy link
Contributor

@IgorMinar IgorMinar left a 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.

@IgorMinar IgorMinar added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Mar 29, 2018
@ngbot
Copy link

ngbot bot commented Mar 29, 2018

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure missing required status "ci/circleci: build"
    failure missing required status "ci/circleci: lint"

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@IgorMinar
Copy link
Contributor

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.

@alxhub alxhub closed this in 9f7bd8f Mar 29, 2018
alxhub pushed a commit that referenced this pull request Mar 29, 2018
alxhub pushed a commit that referenced this pull request Mar 29, 2018
@jbogarthyde jbogarthyde deleted the jb-rxjs-example-updates branch April 27, 2018 17:10
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants