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): update TodoList tutorial #4126

Merged
merged 1 commit into from
Nov 15, 2019
Merged

fix(docs): update TodoList tutorial #4126

merged 1 commit into from
Nov 15, 2019

Conversation

dhmlau
Copy link
Member

@dhmlau dhmlau commented Nov 14, 2019

Fixes #3742

Now that we have lb4 relation command, we need to update the TodoList tutorial to remove the manual steps because those are taken care by the relation generator command.

  • I've shuffled around the content a bit so a good portion of it are existing content.
  • The TodoList tutorial also has the hasOne portion but since we don't have the hasOne option available on lb4 relation (see Add hasOne relation type to lb4 relation #2980), I didn't update the tutorial to include that.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

Copy link
Contributor

@agnes512 agnes512 left a comment

Choose a reason for hiding this comment

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

Thanks for adding the usage of relation generator to the tutorial! Mostly LGTM, just have some comments.

docs/site/sidebars/lb4_sidebar.yml Outdated Show resolved Hide resolved
#### Inclusion of Related Models

When running the `lb4 relation` command, the prompt below registers the
inclusionResolver for this belongsTo relation for you.
Copy link
Contributor

Choose a reason for hiding this comment

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

inclusionResolver -> inclusionResolver

Copy link
Contributor

Choose a reason for hiding this comment

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

Here it seems we are asking user to run it again.

When running the lb4 relation command, the prompt below registers the
inclusionResolver for this belongsTo relation for you.

We need to rewrite this paragraph in the proper tense.

Perhaps something like:

When we ran the lb4 relation commands above, we accepted the default of Yes to the prompt:

? Allow Order queries to include data from related Customer instances? (Y/n)

This registered the inclusionResolver for the relation(s) you were working with above.

Make sure to chose yes ...

docs/site/sidebars/lb4_sidebar.yml Outdated Show resolved Hide resolved

#### Relation decorators

When specifying the `hasMany` relation, it adds `@hasMany()` decorator defines
Copy link
Contributor

Choose a reason for hiding this comment

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

When specifying the hasMany relation, it adds @hasMany() decorator defines this property.

What are we trying to say here?

Copy link
Member Author

Choose a reason for hiding this comment

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

trying to say when running the lb4 relation command, the decorator was added to the property. Let me do some rewording here.

#### Inclusion of Related Models

When running the `lb4 relation` command, the prompt below registers the
inclusionResolver for this belongsTo relation for you.
Copy link
Contributor

Choose a reason for hiding this comment

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

Here it seems we are asking user to run it again.

When running the lb4 relation command, the prompt below registers the
inclusionResolver for this belongsTo relation for you.

We need to rewrite this paragraph in the proper tense.

Perhaps something like:

When we ran the lb4 relation commands above, we accepted the default of Yes to the prompt:

? Allow Order queries to include data from related Customer instances? (Y/n)

This registered the inclusionResolver for the relation(s) you were working with above.

Make sure to chose yes ...

docs/site/sidebars/lb4_sidebar.yml Outdated Show resolved Hide resolved

On the other end,
[`BelongsToAccessor`](https://loopback.io/doc/en/lb4/apidocs.repository.belongstoaccessor.html)
also comes with an inclusion resolver property that we can register on the
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: I feel a more accurate word is function instead of property.

Copy link
Member Author

Choose a reason for hiding this comment

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

changed it to function.

When specifying the `hasMany` relation, it adds `@hasMany()` decorator defines
this property. As the decorator's name suggests, `@hasMany()` informs LoopBack 4
that a todo list can have many todo items.
When we added the `hasMany` relation using the `lb4 relation` command, it added
Copy link
Contributor

Choose a reason for hiding this comment

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

When we added the hasMany relation using the lb4 relation command, it added
@hasMany() decorator defines this property

When we added the hasMany relation using the lb4 relation command, it added the
@hasMany() decorator which defines this property . (Is this what we want to say?)

Copy link
Member Author

@dhmlau dhmlau Nov 15, 2019

Choose a reason for hiding this comment

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

actually, it added the property + the decorator. How about:

When we added the hasMany relation using the lb4 relation command, it added the
@hasMany() decorator together with the todo property.

Copy link
Contributor

Choose a reason for hiding this comment

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

actually, it added the property + the decorator. How about:

When we added the hasMany relation using the lb4 relation command, it added the
@hasMany() decorator together with the todo property.

todo or todos ? The line below says:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we still need a the before '@hasmany decorator'

Copy link
Member Author

Choose a reason for hiding this comment

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

it should be todos property (the one in the documentation is correct).

OK. how about this:

When we added the hasMany relation using the lb4 relation command, it added
the @hasMany() decorator together with the todos property.

@dhmlau
Copy link
Member Author

dhmlau commented Nov 15, 2019

@emonddr, please see my latest commit 24e80aa that addressed your comment. Thanks!

When specifying the `hasMany` relation, it adds `@hasMany()` decorator defines
this property. As the decorator's name suggests, `@hasMany()` informs LoopBack 4
that a todo list can have many todo items.
When we added the `hasMany` relation using the `lb4 relation` command, it added
Copy link
Contributor

Choose a reason for hiding this comment

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

actually, it added the property + the decorator. How about:

When we added the hasMany relation using the lb4 relation command, it added the
@hasMany() decorator together with the todo property.

todo or todos ? The line below says:

image

When specifying the `hasMany` relation, it adds `@hasMany()` decorator defines
this property. As the decorator's name suggests, `@hasMany()` informs LoopBack 4
that a todo list can have many todo items.
When we added the `hasMany` relation using the `lb4 relation` command, it added
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we still need a the before '@hasmany decorator'

@dhmlau
Copy link
Member Author

dhmlau commented Nov 15, 2019

@emonddr, I've pushed my changes and squashed all the commits. PTAL again. Thanks!

Copy link
Contributor

@emonddr emonddr left a comment

Choose a reason for hiding this comment

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

:)

@dhmlau dhmlau merged commit 947835d into master Nov 15, 2019
@dhmlau dhmlau deleted the relation branch November 15, 2019 16:11
@dhmlau dhmlau self-assigned this Nov 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docs: Update TodoList example and model relations pages to use lb4 relation command
4 participants