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.

Sign up for GitHub

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

Jump to bottom

docs: refactor style guide example 03-06 #24996

Closed
wants to merge 1 commit into from
Closed

docs: refactor style guide example 03-06 #24996

wants to merge 1 commit into from

Conversation

danielsogl
Copy link
Contributor

@danielsogl danielsogl commented Jul 20, 2018 •
edited

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?

Style guide example 03-06 uses the old Angular Http module

What is the new behavior?

Updated the example to use thew new HttpClient

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@danielsogl
Copy link
Contributor Author

cc @brandonroberts

@brandonroberts brandonroberts added comp: docs target: patch This PR is targeted for the next patch release labels Jul 23, 2018
@jenniferfell jenniferfell added state: community Someone from the Angular community is working on this issue or submitted this PR and removed state: community Someone from the Angular community is working on this issue or submitted this PR labels Jul 23, 2018
@brandonroberts brandonroberts added the action: merge The PR is ready for merge by the caretaker label Jul 25, 2018
@brandonroberts brandonroberts self-assigned this Jul 25, 2018
vicb pushed a commit that referenced this pull request Jul 25, 2018
@danielsogl @vicb
docs: refactor style guide example 03-06 (#24996)
93d325c 
PR Close #24996
@vicb vicb closed this in 65e18dc Jul 25, 2018
vicb added a commit that referenced this pull request Jul 25, 2018
@vicb
Revert "docs: refactor style guide example 03-06 (#24996)"
01b6aad 
This reverts commit 93d325c.
vicb added a commit that referenced this pull request Jul 25, 2018
@vicb
Revert "docs: refactor style guide example 03-06 (#24996)"
d4ac969 
This reverts commit 65e18dc.
@vicb
Copy link
Contributor

vicb commented Jul 25, 2018

Failing the CI with the error:

ERROR in src/03-06/app/heroes/shared/hero.service.avoid.ts(24,32): error TS2339: Property 'data' does not exist on type 'Hero'.
src/03-06/app/heroes/shared/hero.service.avoid.ts(29,32): error TS2339: Property 'data' does not exist on type 'Hero[]'.

@vicb vicb reopened this Jul 25, 2018
@vicb vicb added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Jul 25, 2018
@danielsogl
Copy link
Contributor Author

@vicb should be fixed now

gkalpak
gkalpak requested changes Jul 25, 2018
View reviewed changes
aio/content/examples/styleguide/src/03-06/app/heroes/shared/hero.service.avoid.ts Outdated
map(response => response.json().data as Hero));
return this.http
.get<Hero>(`api/heroes/${id}`)
.pipe(map((response: any) => response.data as Hero));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

response should already be a Hero, afaict.

aio/content/examples/styleguide/src/03-06/app/heroes/shared/hero.service.avoid.ts Outdated
private http: Http
) { }
private http: HttpClient
) {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep the space (for consistency).

aio/content/examples/styleguide/src/03-06/app/heroes/shared/hero.service.avoid.ts Outdated
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the imports, the rest of this file should be identical to hero.service.ts.

aio/content/examples/styleguide/src/03-06/app/heroes/shared/hero.service.ts Outdated
return this.http.get(`api/heroes`).pipe(
map(response => response.json() as Hero[]));
return this.http.get<Hero[]>(`api/heroes`).pipe(
map(response => response as Hero[]));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pipe(map(...)) is no longer necessary (here and above).

@danielsogl
Copy link
Contributor Author

@gkalpak Fixed

gkalpak
gkalpak approved these changes Jul 25, 2018
View reviewed changes
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One last nit. LGTM otherwise (assuming CI is green) 👍

aio/content/examples/styleguide/src/03-06/app/heroes/shared/hero.service.avoid.ts Outdated
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line should stay 😇

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I open the file there is a free line after the last curly bracket and in GitHub Web too.

Copy link
Member

@gkalpak gkalpak Jul 25, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but this is the line before the last curly 😁

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh damit 😅

@danielsogl
docs: refactor style guide example 03-06
c516eab 
docs: refactor style guide example 03-06


docs: refactor style guide example 03-06


docs: refactor style guide example 03-06
@brandonroberts brandonroberts added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Jul 25, 2018
@vicb
Copy link
Contributor

vicb commented Jul 25, 2018

Thanks for the update, merging again.

@vicb vicb closed this in cf81823 Jul 25, 2018
vicb pushed a commit that referenced this pull request Jul 25, 2018
@danielsogl @vicb
docs: refactor style guide example 03-06 (#24996)
ae7b96b 
docs: refactor style guide example 03-06


docs: refactor style guide example 03-06


docs: refactor style guide example 03-06


PR Close #24996
@danielsogl danielsogl deleted the refactor_03-06 branch July 25, 2018 16:01
@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.
Reviewers

@gkalpak gkalpak gkalpak approved these changes

@brandonroberts brandonroberts brandonroberts approved these changes

Assignees

@brandonroberts brandonroberts

Labels
action: merge The PR is ready for merge by the caretaker cla: yes state: community Someone from the Angular community is working on this issue or submitted this PR target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@danielsogl @vicb @brandonroberts @gkalpak @googlebot @jenniferfell