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(data): add tour of heroes demo app #3131

Closed

Conversation

yharaskrik
Copy link
Contributor

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
[x] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Closes #3004

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Did do too much rewriting of the app, mostly just a port from the original repo.

@ngrxbot
Copy link
Collaborator

ngrxbot commented Aug 30, 2021

Preview docs changes for f5b2e51 at https://previews.ngrx.io/pr3131-f5b2e513/

@markostanimirovic
Copy link
Member

@yharaskrik

I think we need Tour of Heroes app with @ngrx/data included, not the starter version. 🤔 Version with @ngrx/data is on finish branch of ngrx-data-lab repo.

@yharaskrik
Copy link
Contributor Author

Oh no did I copy the wrong app. Uh oh haha

@yharaskrik
Copy link
Contributor Author

@markostanimirovic updated! my bad!

@yharaskrik
Copy link
Contributor Author

Hmm somehow nx.json was formatted but nx format doesn't change it back, should I go in there and edit the file to remove that formatting?

Copy link
Member

@markostanimirovic markostanimirovic 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 Jay! 🥇

Most of the suggestions are related to enabling strict mode 😅

],
declarations: [ModalComponent, ToolbarComponent],
exports: [ToolbarComponent],
entryComponents: [ModalComponent],
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
entryComponents: [ModalComponent],

no longer needed with Ivy :)

@@ -0,0 +1,5 @@
export class Hero {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export class Hero {
export interface Hero {

@@ -0,0 +1,5 @@
export class Villain {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export class Villain {
export interface Villain {

@@ -3,7 +3,8 @@
"compilerOptions": {
"outDir": "../../dist/out-tsc",
"module": "commonjs",
"types": ["jest", "node"]
"types": ["jest", "node"],
"strict": false
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"strict": false

😁

parsed.collectionName =
this.active && isDefaultRoot
? mapCollectionName(parsed.collectionName)
: undefined;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
: undefined;
: '';

@Output() add = new EventEmitter<Villain>();
@Output() update = new EventEmitter<Villain>();

@ViewChild('name', { static: true }) nameElement: ElementRef;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@ViewChild('name', { static: true }) nameElement: ElementRef;
@ViewChild('name', { static: true }) nameElement!: ElementRef;

Comment on lines +18 to +19
@Input() villains: Villain[];
@Input() selectedVillain: Villain;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@Input() villains: Villain[];
@Input() selectedVillain: Villain;
@Input() villains: Villain[] = [];
@Input() selectedVillain: Villain | Record<string, never> | null = null;

@@ -0,0 +1,54 @@
import { Component, OnInit } from '@angular/core';
import { Observable } from 'rxjs';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { Observable } from 'rxjs';

Comment on lines +12 to +19
loading$: Observable<boolean>;
selected: Villain;
villains$: Observable<Villain[]>;

constructor(private villainService: VillainService) {
this.villains$ = villainService.entities$;
this.loading$ = villainService.loading$;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
loading$: Observable<boolean>;
selected: Villain;
villains$: Observable<Villain[]>;
constructor(private villainService: VillainService) {
this.villains$ = villainService.entities$;
this.loading$ = villainService.loading$;
}
villains$ = this.villainService.entities$;
loading$ = this.villainService.loading$;
selected: Villain | Record<string, never> | null = null;
constructor(private villainService: VillainService) {}

}

enableAddMode() {
this.selected = <any>{};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.selected = <any>{};
this.selected = {};

@yharaskrik
Copy link
Contributor Author

Haha thanks Marko! I'll work on this tonight. I think the other example app has strict disabled so figured it was fine for here as well. I did do a literally copy paste so there's probably a lot of easy cleanup to be done.

@brandonroberts
Copy link
Member

@yharaskrik will you fix the merge conflicts here also? Thanks!

@yharaskrik
Copy link
Contributor Author

Yup! I still gotta work on Markos suggestions too it seems, work has a strangle hold on me right now! Ill get the changes and conflicts done soon hopefully.

@markostanimirovic markostanimirovic added the Needs Cleanup Review changes needed label Oct 8, 2021
@brandonroberts
Copy link
Member

Going to close this one for now. Feel free to reopen or open a new one when ready

@markostanimirovic
Copy link
Member

@yharaskrik Would you like to re-open this PR with applied suggestions? 👀

If you don't have time, I can take care of it.

@yharaskrik
Copy link
Contributor Author

@yharaskrik Would you like to re-open this PR with applied suggestions? 👀

If you don't have time, I can take care of it.

I can take a look! Although it does not seem that I can reopen it! Can you reopen for me please and thanks!

@timdeschryver timdeschryver reopened this Aug 16, 2022
@timdeschryver
Copy link
Member

I'm closing this again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Cleanup Review changes needed
Projects
None yet
5 participants