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: add http guide sample and adjust text #21326

Closed
wants to merge 1 commit into from

Conversation

wardbell
Copy link
Contributor

@wardbell wardbell commented Jan 5, 2018

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?

The current Http Guide does not have a working code sample. Worse, the code example for this guide is actually the code sample for the discontinued old HTTP module.

The guide does not reference that sample. Instead, it uses markdown ticks to present extracts from a hypothetical application. These extracts do not conform to style guidelines (e.g, they perform HTTP operations within the components instead of delegating to a service) and In several cases there are minor syntax errors.

What is the new behavior?

Added a working HttpClient sample that is Hero themed and loosely based on ToH part 6 (the HTTP lesson).

The guide prose is adjusted to incorporate excerpts from the code sample. All substantive points in the original prose are retained.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@mary-poppins
Copy link

You can preview a3de0fe at https://pr21326-a3de0fe.ngbuilds.io/.
You can preview a53d83f at https://pr21326-a53d83f.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 6636ebe at https://pr21326-6636ebe.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 4912013 at https://pr21326-4912013.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 40f81fe at https://pr21326-40f81fe.ngbuilds.io/.

@wardbell wardbell force-pushed the http-new-sample branch 2 times, most recently from 07d5302 to 4f8e051 Compare January 6, 2018 22:42
@mary-poppins
Copy link

You can preview 07d5302 at https://pr21326-07d5302.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 4f8e051 at https://pr21326-4f8e051.ngbuilds.io/.

@wardbell wardbell force-pushed the http-new-sample branch 2 times, most recently from c4b6725 to f9c029a Compare January 7, 2018 11:47
@mary-poppins
Copy link

You can preview c4b6725 at https://pr21326-c4b6725.ngbuilds.io/.

@mary-poppins
Copy link

You can preview f9c029a at https://pr21326-f9c029a.ngbuilds.io/.

@mary-poppins
Copy link

You can preview edaa027 at https://pr21326-edaa027.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 5c13889 at https://pr21326-5c13889.ngbuilds.io/.

@wardbell wardbell force-pushed the http-new-sample branch 2 times, most recently from f602f33 to 9c29903 Compare January 9, 2018 04:07
@mary-poppins
Copy link

You can preview f602f33 at https://pr21326-f602f33.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 9c29903 at https://pr21326-9c29903.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 12eb0db at https://pr21326-12eb0db.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 7569265 at https://pr21326-7569265.ngbuilds.io/.

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.

Can we get the stackblitz PR #20165 merged first before this lands? otherwise the stackblitz PR will suffer another huge merge conflict.

@wardbell
Copy link
Contributor Author

I'm cool with fitting this in after StackBlitz lands. I anticipated that a bit by preparing the stackblitz.json files for this guide.

Sign me up for the conflict resolution when the time comes.

@wardbell
Copy link
Contributor Author

The merge conflicts on Jan 24 are due to the Stackblitz merge. I will resolve and move to stackblitz at same time.

@wardbell
Copy link
Contributor Author

wardbell commented Jan 30, 2018

Merge conflicts created by stackblitz PR resolved.
Stackblitz's for app and tests both working.
E2E tests and lint check pass
Docs are good and pass link check.

Ready to merge.

@mary-poppins
Copy link

You can preview 35ed0fe at https://pr21326-35ed0fe.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 4bd2c96 at https://pr21326-4bd2c96.ngbuilds.io/.

@wardbell
Copy link
Contributor Author

wardbell commented Jan 30, 2018

The CI failure (as of this comment) appears to be a known, reported, unrelated saucelabs flake.

@petebacondarwin petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed state: blocked labels Jan 31, 2018
@ngbot
Copy link

ngbot bot commented Jan 31, 2018

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure missing required label: "PR target:"
    pending status "code-review/pullapprove" is pending

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.

@petebacondarwin petebacondarwin added the target: patch This PR is targeted for the next patch release label Jan 31, 2018
@jasonaden jasonaden closed this in 7a20691 Jan 31, 2018
jasonaden pushed a commit that referenced this pull request Jan 31, 2018
@wardbell wardbell deleted the http-new-sample branch January 31, 2018 19:36
jbogarthyde pushed a commit to jbogarthyde/angular that referenced this pull request Feb 23, 2018
leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018
@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 target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants