Answer:1 #778

chekit · 2024-04-16T12:18:28Z

Checklist for challenge submission

[x ] Start your PR message with Answer:${challenge_number}

Warning:

If you want feedback or review, you must support the project on GitHub:
- $5 per review
- $25 for lifetime reviews
- Create one of more challenges
- Have many valuable contributions to the project

Alternatively, you can still submit your PR to join the list of answered challenges or to be reviewed by a community member. 🔥

GregOnNet

Hi @chekit,

thank you so much for your PR. 🙏🏻
This is the first review, I do.
So please do not take my comments too serious.

Here are general thoughts about the given goal “. The goal of the challenge is to create a CardComponent that can be customized without any modifications.”

I think the typing of the list-input of CardComponent would cause an issue, since the typing limits the usage to objects having id, firstname, name.
Other than that, I think the intention was to make the card reusable by using ngTemplateOutlet
Nevertheless, you managed to make the card reusable by abstracting the data-model.
In the end, I think ngTemplateOutlet makes the card more reusable.

Btw:
I am not sure if I should approve the PR or not.
Not sure about the process, here. :-) //cc @tomalaforge

Cheers from Leipzig, Germany
Gregor

GregOnNet · 2024-04-17T07:04:41Z

apps/angular/projection/src/app/component/city-card/city-card.component.ts

-  template: 'TODO City',
+  template: `
+    <app-card
+      [list]="data$ | async"


Cool that you use a stream here.
You should be able to use ChangeDetectionStrategy.OnPush here.

#performance #no-blocker

Definitely make sense 👍🏻

GregOnNet · 2024-04-17T07:05:35Z

apps/angular/projection/src/app/component/city-card/city-card.component.ts

+  template: `
+    <app-card
+      [list]="data$ | async"
+      (addNewItem)="onAddNewItem()"


Instead of onAddNewItem you could be more specific about the handler → addNewCitiy().

#personalPerference #no-blocker

Yeah, as you noticed later the intention was to move all repetitive code out of components, to make it possible to handle it in one place.

GregOnNet · 2024-04-17T07:09:38Z

apps/angular/projection/src/app/component/city-card/city-card.component.ts

+      [list]="data$ | async"
+      (addNewItem)="onAddNewItem()"
+      (deleteItem)="onDeleteItem($event)"
+      [backgroundColor]="'rgba(250, 10, 250, 0.1)'">


I think this is better than using ::ng-deep.

We could also think of allowing to pass a class-name: backgroundClass="bg-red-200"

This allows to reuse styles from an existing CSS-Framework like prime flex or tailwind.

#idea #no-blocker

I've left it with style, but with refactored implementation class could be easily implemented as well :)

GregOnNet · 2024-04-17T07:12:57Z

apps/angular/projection/src/app/component/teacher-card/teacher-card.component.ts


-    this.store.teachers$.subscribe((t) => (this.teachers = t));
+  override onAddNewItem() {


OK, now I understand the “generic” naming.

From my experience, this adds another layer of indirection that does not provide a benefit for the developer.
This approach makes absolute sense to me if we are in the area of spinning up components dynamically from a configuration. Then we need to introduce contracts and hooks.

The idea is to move all repeated functionality to abstract class (DRY), thus you don't need to copy paste it once you create a new card type component and focus only on the things that differs this component from others.

The idea is to move all repeated functionality to abstract class (DRY), thus you don't need to copy paste it once you create a new card type component and focus only on the things that differs this component from others.

Yes, I understand the goal. As far as I can tell, this approach only works in very few situations.
Often we tend to abstract too early. Then a business requirement changes, and we start adding conditions to our abstractions in order to make them work for a specific case.

That's why I value SRP (Single Responsibility Principle) over DRY.
In our challenge here, I would ask, what is a card responsible for? Aha, providing a visual boundary and a spot to render
any content.

GregOnNet · 2024-04-17T07:16:48Z

apps/angular/projection/src/app/ui/card/card.component.ts

        Add
      </button>
    </div>
  `,
+  styles: `
+    .card__image {
+      width: 200px;


Why do we need to hard-code the width of the image here, if our goal is to have an abstract card-implementation?

#api #no-blocker

Indeed, put this back to the specific card as it was initially.

GregOnNet · 2024-04-17T07:20:43Z

apps/angular/projection/src/app/ui/card/card.component.ts

-        src="assets/img/student.webp"
-        width="200px" />
-
+      [bgColor]="backgroundColor()">


If we allow passing a CSS-class, we do not need the custom BgColorDirective.

#no-blocker #less-code-to-maintain

Yeah, now I see what you mean. It is definitely look like an overhead.

GregOnNet · 2024-04-17T07:22:53Z

apps/angular/projection/src/app/ui/card/card.component.ts

-  @Input() list: any[] | null = null;
-  @Input() type!: CardType;
-  @Input() customClass = '';
+  list = input<{ id: number; firstName?: string; name?: string }[] | null>(


This typing limits the data structure we can pass to the card.
How would we act if we would like to use this card for displaying a Product having no firstname.

Maybe it is possible to make CardComponent generic: CardComponent<T>

#api

You are right. With the ngTemplateOutlet approach it is not need to be too specific for list property.

…dundant directive

chekit · 2024-04-19T09:22:20Z

Hi @chekit,

thank you so much for your PR. 🙏🏻 This is the first review, I do. So please do not take my comments too serious.

Here are general thoughts about the given goal “. The goal of the challenge is to create a CardComponent that can be customized without any modifications.”

I think the typing of the list-input of CardComponent would cause an issue, since the typing limits the usage to objects having id, firstname, name.

Other than that, I think the intention was to make the card reusable by using ngTemplateOutlet

Nevertheless, you managed to make the card reusable by abstracting the data-model.

In the end, I think ngTemplateOutlet makes the card more reusable.

Btw: I am not sure if I should approve the PR or not. Not sure about the process, here. :-) //cc @tomalaforge

Cheers from Leipzig, Germany Gregor

Hi @GregOnNet,

Thank you a lot for such a detailed review. It was really nice to practice and recall the concepts which I'm not using everyday. I did some changes and left a comments to your comments.

Once again, much appreciation :)

P.S.
I'm not sure you need to approve PR to not override the original exercise

Cheers from Amsterdam, The Netherlands. Anton 😉

GregOnNet · 2024-04-22T07:06:46Z

@chekit Hey, thanks for the feedback. I really appreciate it.

Yes, I learned, that these PRs are tagged in order to mark them as answer. :-)

github-actions · 2024-05-09T00:34:26Z

This pull request is stale because it has been open for 15 days with no activity.

github-actions · 2024-05-15T00:34:33Z

This pull request was closed because it has been inactive for 5 days since being marked as stale.

Anton Cherepov added 2 commits April 16, 2024 11:59

feat: add abstract class for reusable parts, implement city card

c8b6fde

refactor: remove redundant extra input param

d265e05

tomalaforge added answer answer 1 exercice projection labels Apr 16, 2024

GregOnNet reviewed Apr 17, 2024

View reviewed changes

Anton Cherepov added 6 commits April 18, 2024 12:03

refactor: add on push strategy

ba7cdda

fix: add customCLass property back, set bg style via input, remove re…

e43e02a

…dundant directive

refactor: add contract for card component

2c13ba8

refactor: card component read out ng-template

81858ee

refactor: remove extra directive

7bf7a90

refactor: cleanup

8eb9b50

github-actions bot added the stale label May 9, 2024

github-actions bot closed this May 15, 2024

tomalaforge removed the stale label May 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Answer:1 #778

Answer:1 #778

chekit commented Apr 16, 2024

GregOnNet left a comment •

edited

Loading

GregOnNet Apr 17, 2024

chekit Apr 19, 2024

GregOnNet Apr 17, 2024

chekit Apr 19, 2024

GregOnNet Apr 17, 2024

chekit Apr 19, 2024

GregOnNet Apr 17, 2024

chekit Apr 19, 2024

GregOnNet Apr 22, 2024

GregOnNet Apr 17, 2024

chekit Apr 19, 2024

GregOnNet Apr 17, 2024

chekit Apr 19, 2024

GregOnNet Apr 17, 2024

chekit Apr 19, 2024

chekit commented Apr 19, 2024

GregOnNet commented Apr 22, 2024

github-actions bot commented May 9, 2024

github-actions bot commented May 15, 2024


		this.store.teachers$.subscribe((t) => (this.teachers = t));
		override onAddNewItem() {

Answer:1 #778

Answer:1 #778

Conversation

chekit commented Apr 16, 2024

Checklist for challenge submission

Warning:

GregOnNet left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chekit commented Apr 19, 2024

GregOnNet commented Apr 22, 2024

github-actions bot commented May 9, 2024

github-actions bot commented May 15, 2024

GregOnNet left a comment •

edited

Loading