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(docs-infra): clarify toh-2 error message #45878

Conversation

dario-piotrowicz
Copy link
Contributor

the second step of the tour of heroes refers to a runtime error which
generally isn't presented to new users since it gets caught by the
TypeScript compiler's strict mode, clarify such detail so not to confuse
readers

resolves #45759

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:

Issue

Issue Number: #45759

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

@dario-piotrowicz thanks for the PR! I've left some comments and I also noticed that the lint CI job is failing. Could you please take a look when you get a chance? Thank you.

aio/content/tutorial/toh-pt2.md Outdated Show resolved Hide resolved
aio/content/tutorial/toh-pt2.md Outdated Show resolved Hide resolved
aio/content/tutorial/toh-pt2.md Outdated Show resolved Hide resolved
aio/content/tutorial/toh-pt2.md Outdated Show resolved Hide resolved
aio/content/tutorial/toh-pt2.md Outdated Show resolved Hide resolved
aio/content/tutorial/toh-pt2.md Outdated Show resolved Hide resolved
aio/content/tutorial/toh-pt2.md Outdated Show resolved Hide resolved
aio/content/tutorial/toh-pt2.md Outdated Show resolved Hide resolved
@AndrewKushnir
Copy link
Contributor

@dario-piotrowicz the lint error is a bit weird: it complains about a file that you didn't change in this PR... May be try to rebase on top of the latest main?

@AndrewKushnir AndrewKushnir added comp: docs 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 May 5, 2022
@ngbot ngbot bot modified the milestone: Backlog May 5, 2022
@dario-piotrowicz dario-piotrowicz force-pushed the aio-toh-2-inaccurate-error-message branch from 5f257c0 to a982cfa Compare May 5, 2022 22:25
@dario-piotrowicz
Copy link
Contributor Author

@AndrewKushnir, I've addressed your comments, thank you so very much for the great review 😍

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

I wanted to use ng-container for the *ngIf here but I thought that I would not overwhelm newcomers so kept the div what do you think? is the right call?

If I understand correctly, the <div> was already there, right? Anyway, I think it's a good idea to use a <div> during this part of the tutorial and introduce <ng-container> later.

Above in the tutorial we only say not to assign a value to the selectedHero, but we never directly say that its value is going to be undefined here I do however use the undefined keyword, I hope that it cannot lead to confusion?

I've added a suggestion to rephrase that section. Please let me know what you think.

Should I add a short explanation about how *ngIf works with truthy/falsy values? or is that clear?

I think it's clear and we can expend on it more if we get some feedback.

aio/content/tutorial/toh-pt2.md Outdated Show resolved Hide resolved
aio/content/tutorial/toh-pt2.md Outdated Show resolved Hide resolved
@dario-piotrowicz
Copy link
Contributor Author

dario-piotrowicz commented May 6, 2022

@AndrewKushnir thanks a lot again for the nice reviewing! 😄 , I've applied the suggestions 🙂

AndrewKushnir
AndrewKushnir previously approved these changes May 7, 2022
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

@dario-piotrowicz thanks a lot for addressing the feedback! 👍

@AndrewKushnir AndrewKushnir 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 May 7, 2022
the second step of the tour of heroes refers to a runtime error which
generally isn't presented to new users since it gets caught by the
TypeScript compiler's strict mode, clarify such detail so not to confuse
readers

resolves angular#45759
@dario-piotrowicz
Copy link
Contributor Author

@dario-piotrowicz thanks a lot for addressing the feedback! +1

My pleasure! thanks again for the awesome reviewing! 😊

@AndrewKushnir
Copy link
Contributor

This PR was merged into the repository by commit b6156f4.

AndrewKushnir pushed a commit that referenced this pull request May 9, 2022
the second step of the tour of heroes refers to a runtime error which
generally isn't presented to new users since it gets caught by the
TypeScript compiler's strict mode, clarify such detail so not to confuse
readers

resolves #45759

PR Close #45878
AndrewKushnir pushed a commit that referenced this pull request May 9, 2022
the second step of the tour of heroes refers to a runtime error which
generally isn't presented to new users since it gets caught by the
TypeScript compiler's strict mode, clarify such detail so not to confuse
readers

resolves #45759

PR Close #45878
@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 Jun 9, 2022
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 target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inaccurate error message on step 2 of the tutorial
2 participants