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

fix(docs-infra): convert hard-coded comparing-observables examples into a proper mini-app #34327

Conversation

sonukapoor
Copy link
Contributor

…stable files

It also updates some indention issues with the code that is part of inline tables.

Fixes #31024

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
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #31024

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@sonukapoor sonukapoor requested review from IgorMinar and a team as code owners December 10, 2019 14:50
@ngbot ngbot bot added this to the needsTriage milestone Dec 11, 2019
@gkalpak gkalpak added aio: preview area: build & ci Related the build and CI infrastructure of the project action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Dec 12, 2019
@mary-poppins
Copy link

You can preview dc19af6 at https://pr34327-dc19af6.ngbuilds.io/.
You can preview fd0d3ac at https://pr34327-fd0d3ac.ngbuilds.io/.

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.

Off to a great start 🎉
Some more fine-tuning and this should be good to go!

(BTW, why the empty main.ts file?)

aio/content/guide/comparing-observables.md Show resolved Hide resolved
aio/content/guide/comparing-observables.md Show resolved Hide resolved
aio/content/guide/comparing-observables.md Show resolved Hide resolved
aio/content/guide/comparing-observables.md Outdated Show resolved Hide resolved
@gkalpak gkalpak added state: WIP and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 12, 2019
@sonukapoor sonukapoor requested a review from a team as a code owner December 12, 2019 21:06
@mary-poppins
Copy link

You can preview 9def4e8 at https://pr34327-9def4e8.ngbuilds.io/.

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.

Shaping up nicely ✨
I left a few more comments (nothing major).

aio/angular.json Outdated Show resolved Hide resolved
aio/angular.json Outdated Show resolved Hide resolved
aio/content/examples/comparing-observables/src/promises.ts Outdated Show resolved Hide resolved
aio/content/examples/comparing-observables/src/promises.ts Outdated Show resolved Hide resolved
aio/content/guide/comparing-observables.md Outdated Show resolved Hide resolved
aio/content/guide/comparing-observables.md Show resolved Hide resolved
aio/content/guide/comparing-observables.md Outdated Show resolved Hide resolved
@mary-poppins
Copy link

You can preview dd1e075 at https://pr34327-dd1e075.ngbuilds.io/.

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.

Almost there 🎉

Can you please take care of #34327 (comment) (rename the files to plural and the docregion to singular), squash the commits into one and update the commit message to something along the lines of dd9793f?

@sonukapoor sonukapoor force-pushed the converts-hard-coded-examples-to-proper-examples branch from dd1e075 to 5763d55 Compare December 16, 2019 15:08
@sonukapoor
Copy link
Contributor Author

@gkalpak I believe this resolves everything. LMK if anything is missing.

@mary-poppins
Copy link

You can preview 5763d55 at https://pr34327-5763d55.ngbuilds.io/.

@sonukapoor sonukapoor force-pushed the converts-hard-coded-examples-to-proper-examples branch from 5763d55 to b7ebe78 Compare December 16, 2019 16:22
@gkalpak gkalpak changed the title fix(docs-infra): convert hard-coded rxjs examples into proper mini-te… fix(docs-infra): convert hard-coded comparing-observables examples into a proper mini-app Dec 16, 2019
@sonukapoor sonukapoor force-pushed the converts-hard-coded-examples-to-proper-examples branch from b7ebe78 to 70e85d1 Compare December 16, 2019 16:35
@gkalpak gkalpak added action: review The PR is still awaiting reviews from at least one requested reviewer and removed state: WIP labels Dec 16, 2019
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.

Great work, @sonukapoor! Thx 💯

@sonukapoor sonukapoor force-pushed the converts-hard-coded-examples-to-proper-examples branch from 70e85d1 to 24a7a71 Compare December 16, 2019 16:45
…into a proper mini-app

Previously, the examples in the `comparing-observables` guide were hard-coded.
This made it impossible to test them and verify they are correct.

This commit fixes this by converting them into a proper mini-app. In a
subsequent commit, tests will be added to verify that the source code
works as expected (and guard against regressions).

Fixes angular#31024
@sonukapoor sonukapoor force-pushed the converts-hard-coded-examples-to-proper-examples branch from 24a7a71 to 692bf9e Compare December 16, 2019 17:16
@gkalpak gkalpak added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 16, 2019
@mary-poppins
Copy link

You can preview 692bf9e at https://pr34327-692bf9e.ngbuilds.io/.

@kara kara added the action: review The PR is still awaiting reviews from at least one requested reviewer label Dec 16, 2019
@sonukapoor
Copy link
Contributor Author

@IgorMinar Can you please review this?

@atscott atscott removed the action: merge The PR is ready for merge by the caretaker label Jan 10, 2020
Copy link
Member

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

LGTM

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 great! Thank you

@IgorMinar IgorMinar added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 19, 2020
@gkalpak gkalpak added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Jan 19, 2020
@gkalpak
Copy link
Member

gkalpak commented Jan 19, 2020

@caretaker: Global approval for the docs-only changes.

@matsko matsko closed this in def4127 Jan 21, 2020
matsko pushed a commit that referenced this pull request Jan 21, 2020
…into a proper mini-app (#34327)

Previously, the examples in the `comparing-observables` guide were hard-coded.
This made it impossible to test them and verify they are correct.

This commit fixes this by converting them into a proper mini-app. In a
subsequent commit, tests will be added to verify that the source code
works as expected (and guard against regressions).

Fixes #31024

PR Close #34327
@sonukapoor sonukapoor deleted the converts-hard-coded-examples-to-proper-examples branch January 23, 2020 22:29
sonukapoor added a commit to sonukapoor/angular that referenced this pull request Feb 13, 2020
…into a proper mini-app (angular#34327)

Previously, the examples in the `comparing-observables` guide were hard-coded.
This made it impossible to test them and verify they are correct.

This commit fixes this by converting them into a proper mini-app. In a
subsequent commit, tests will be added to verify that the source code
works as expected (and guard against regressions).

Fixes angular#31024

PR Close angular#34327
@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 Feb 23, 2020
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 aio: preview area: build & ci Related the build and CI infrastructure of the project 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: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs: observables examples hard-coded and outdated
9 participants