-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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(data): add document about how customize a entityCollection level property of ngrx-data #1921
docs(data): add document about how customize a entityCollection level property of ngrx-data #1921
Conversation
452be79
to
428e48a
Compare
Preview docs changes for 428e48a at https://previews.ngrx.io/pr1921-428e48a/ |
Thanks for the PR @JiaLiPassion! Something is being formatted incorrectly in the preview starting at bullet 3 that throws off the rest of the sections. |
@brandonroberts, thanks, I fixed it, please review. |
428e48a
to
1776114
Compare
Preview docs changes for 1776114 at https://previews.ngrx.io/pr1921-1776114/ |
1776114
to
1149354
Compare
@timdeschryver, thanks for the review, I have updated the doc! |
Preview docs changes for 1149354 at https://previews.ngrx.io/pr1921-1149354/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went over it one more time and found some small improvements.
Besides these, LGTM and I learned something new 👍
1149354
to
db7d7c6
Compare
@timdeschryver, thank you for the detailed review, I updated the doc! |
db7d7c6
to
6a20c14
Compare
Preview docs changes for db7d7c6 at https://previews.ngrx.io/pr1921-db7d7c6/ |
Preview docs changes for 6a20c14 at https://previews.ngrx.io/pr1921-6a20c14/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I think the typos are fixed.
Not very familiar with ngrx/data, an extra pair of eyes would be helpful here 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few more changes
@wesleygrimes, thanks, sorry, I didn't see the changes you requested, could you check it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jia,
Nice work. Here are my suggestions.
Thanks,
Wes
Sorry about that, they should be there now. |
6a20c14
to
2ce1807
Compare
@wesleygrimes, thanks for the review, I have updated the doc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need spacing between Step and Step Number. For some reason, GitHub would not let me suggest fix on Step3
but that should be fixed as well.
Thanks!
Wes
2ce1807
to
f9325e4
Compare
@wesleygrimes, sure, thanks, I have fixed those 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more round of changes, Thanks Jia!
f9325e4
to
11d52a1
Compare
… property of ngrx-data
11d52a1
to
db19d4e
Compare
Preview docs changes for f9325e4 at https://previews.ngrx.io/pr1921-f9325e4/ |
LGTM |
@wesleygrimes, thanks, I will use |
Preview docs changes for db19d4e at https://previews.ngrx.io/pr1921-db19d4e/ |
@JiaLiPassion I think this needs an update to step 1. The default QUERY_MANY_SUCCESS reducer is expecting action.payload.data to be an array of entities but the QUERY_MANY action action.payload.data will be whatever the api returns. Say it returns interface QueryManyAPIResponse { You can add a property total to the action payload but action.payload.data needs to be the entities if you want the default reducer for a query many success to work.
Also |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Does this PR introduce a breaking change?
Add documentation about how to customize a EntityCollection level property. Such as in my use case, I want to add
paging information
inEntityCollection level
, but currentdocument
can only add an additional property ininitialization
, there is no document about who to get the property from backend.