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 products.ts file on getting started page #34301

Closed

Conversation

ajitsinghkaler
Copy link
Contributor

Fixes #34291

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?

In the current behavior, products.ts page is not visible on the getting started page.

Issue Number: 34291

What is the new behavior?

Products.ts page is visible on the new getting started page.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@ajitsinghkaler
Copy link
Contributor Author

@gkalpak can ou please review this.

@kapunahelewong kapunahelewong added aio: preview comp: docs effort1: hours target: patch This PR is targeted for the next patch release risk: low feature Issue that requests a new feature labels Dec 9, 2019
@mary-poppins
Copy link

You can preview 7c02a49 at https://pr34301-7c02a49.ngbuilds.io/.
You can preview efe649e at https://pr34301-efe649e.ngbuilds.io/.

@ngbot ngbot bot modified the milestone: Backlog Dec 9, 2019
@kapunahelewong kapunahelewong added this to In Progress in docs Dec 9, 2019
Copy link
Contributor

@kapunahelewong kapunahelewong left a comment

Choose a reason for hiding this comment

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

Thank you for this, @ajitsinghkaler. Just one comment.

@@ -80,8 +80,14 @@ To help you get going, the following steps use predefined product data and metho

1. Each product in the list displays the same way, one after another on the page. To iterate over the predefined list of products, put the `*ngFor` directive on a `<div>`, as follows:
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're adding a file, but the reader doesn't need to copy the contents of products.ts into their project, we should point it out. Maybe something like the following would be helpful:

Suggested change
1. Each product in the list displays the same way, one after another on the page. To iterate over the predefined list of products, put the `*ngFor` directive on a `<div>`, as follows:
1. When you generated the app, StackBlitz automatically created a file called `products.ts`, with a list of products to use in your app. Each product in the list displays the same way, one after another on the page. To iterate over the predefined list of products, put the `*ngFor` directive on a `<div>`, as follows:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made the changes please have a look

@mary-poppins
Copy link

You can preview 6a2ff77 at https://pr34301-6a2ff77.ngbuilds.io/.

<code-pane header="src/app/product-list/product-list.component.html" path="getting-started/src/app/product-list/product-list.component.2.html" region="ngfor">
</code-pane>

<code-pane header="src/app/products.ts" path="getting-started/src/app/products.ts"></code-pane>
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I don't think this change addresses #34291. #34291 refers to the sentence:

The product list component also defines a products property that contains imported data for each product from the products array in products.ts.

This sentence appears in the "Input" section, further below, so adding the products.ts file here (before it is being mentioned) would be more confusing imo.

Personally, I don't agree with the suggestion in #34291 that products.ts should be shown in the tutorial just because it is part of the tutorial. There are many files that are part of it, but not shown in the tutorial. All files are available on the StackBlitz project, so the user can look at them if they want to.

In any case, if we are going to include products.ts in a code-pane, it should either be somewhere around the Input section (where it is mentioned) or we should mention it earlier.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed with @gkalpak. I don't think it's necessary to add it, but if we do, it should definitely be mentioned (wherever it may go). I see why @ajitsinghkaler added it here, though, since those are the products we're iterating through.

@ajitsinghkaler what are your thoughts about your change? Do you feel it enhances clarity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kapunahelewong I get what @gkalpak is trying to tell even I was not sure to sow the product.ts file there but my concern is this is the first tutorial we should mostly spoon feed it in there if it was one of the later tutorials it was okay to not show it but I'm not sure about the starter. Or what we can do is add a line in the stack blitz Tips where we explain the starter files. That seems like a more elegant solution

@gkalpak I thought #34291 would be resolved if we show the file once at the top it won't be a problem anymore, showing it the first time it was mentioned should be enough.

I need your thoughts on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a line about it in the StackBlitz notes section could be helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add another line regarding products.ts in the stackblitz notes

@ajitsinghkaler
Copy link
Contributor Author

@kapunahelewong I have made some changes can you please have a look.

@mary-poppins
Copy link

You can preview 628eb3c at https://pr34301-628eb3c.ngbuilds.io/.

@@ -50,6 +50,9 @@ expect, save and then click the refresh button.
* StackBlitz is continually improving, so there may be
slight differences in generated code, but the app's
behavior will be the same.
* The tutorial may contain some file references straight
from the StackBlitz examples. If you can't find some files
please have a look in the examples.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about:

When you generate the StackBlitz example app that accompanies the Getting Started, StackBlitz creates the starter files and mock data for you. The files you'll use throughout the Getting Started are in the src folder of the StackBlitz example app.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, similar message but the emphasis is on where to find the files rather than not being able to find them. Does that help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kapunahelewong I think we can put this as a general message rather than it being just about Getting started example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because there we are giving tips about how to use stackblitz for all example apps I've other text for the products.ts file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense to me. @ajitsinghkaler, could you put src in backticks so it shows up in code font?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kapunahelewong Can you please have a look I put in the backticks

@ajitsinghkaler ajitsinghkaler force-pushed the tutorial-changes branch 2 times, most recently from 88a52bb to 342817b Compare December 12, 2019 03:17
@mary-poppins
Copy link

You can preview 342817b at https://pr34301-342817b.ngbuilds.io/.

in the getting started page (first tutorial) file products.ts which was not shown and was only present in the StackBlitz examples. So added a refrence that it is present in the example and also added a note that examples may carry filenames not present please look at StackBliz examples for details

Fixes angular#34291
@mary-poppins
Copy link

You can preview af02f3d at https://pr34301-af02f3d.ngbuilds.io/.

Copy link
Contributor

@kapunahelewong kapunahelewong left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks, @ajitsinghkaler.

Let's see what @gkalpak has as feedback as our next step.

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.

LGTM2 👍

@ajitsinghkaler
Copy link
Contributor Author

@gkalpak can you please merge this

@gkalpak gkalpak added the action: merge The PR is ready for merge by the caretaker label Dec 17, 2019
@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 Dec 17, 2019
@gkalpak
Copy link
Member

gkalpak commented Dec 17, 2019

@ajitsinghkaler: I didn't realize it didn't have the merge label. Added. Thx for the ping.
(BTW, please do not re-request reviews, unless you really need the person to review the code again. It is confusing.)

@gkalpak
Copy link
Member

gkalpak commented Dec 17, 2019

merge-assistance: Global approval for this docs-only change.

@kapunahelewong kapunahelewong moved this from In Progress to Waiting for Merge in docs Dec 17, 2019
kara pushed a commit that referenced this pull request Dec 17, 2019
in the getting started page (first tutorial) file products.ts which was not shown and was only present in the StackBlitz examples. So added a refrence that it is present in the example and also added a note that examples may carry filenames not present please look at StackBliz examples for details

Fixes #34291

PR Close #34301
@kara kara closed this in 6bfe214 Dec 17, 2019
@kapunahelewong kapunahelewong moved this from Waiting for Merge to Done in docs Dec 17, 2019
@ajitsinghkaler ajitsinghkaler deleted the tutorial-changes branch December 28, 2019 17:49
@ajitsinghkaler ajitsinghkaler restored the tutorial-changes branch December 28, 2019 17:49
@ajitsinghkaler ajitsinghkaler deleted the tutorial-changes branch December 28, 2019 17:50
@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 Jan 28, 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 cla: yes effort1: hours feature Issue that requests a new feature merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note risk: low target: patch This PR is targeted for the next patch release
Projects
docs
Done
Development

Successfully merging this pull request may close these issues.

"products.ts" reference at https://angular.io/start
5 participants