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

refactor: Apply new Angular control flow #1465

Closed
wants to merge 1 commit into from

Conversation

dominik003
Copy link
Collaborator

This PR applies the new Angular control flow introduced with Angular 17 to several components. This includes some simplifications that can now be applied more easily. In addition, some css classes have been converted to tailwind and unused css files have been removed. Last but not least, some line breaks and indentations in the components have been adjusted to improve readability.

Copy link

codecov bot commented Mar 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.94%. Comparing base (b04fb2e) to head (49b3e3c).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1465   +/-   ##
=======================================
  Coverage   76.94%   76.94%           
=======================================
  Files         170      170           
  Lines        5668     5668           
  Branches      650      650           
=======================================
  Hits         4361     4361           
  Misses       1161     1161           
  Partials      146      146           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dominik003 dominik003 force-pushed the refactor-apply-new-angular-control-flow branch from 08d14f7 to 93515ca Compare March 26, 2024 17:00
Copy link
Contributor

@romeonicholas romeonicholas left a comment

Choose a reason for hiding this comment

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

So the changes I'm seeing make sense, though there are a number of logic changes I haven't been able to personally test. I'm kind of wondering what the criteria for updating is, since you left some of the old logic controls in components you updated. Is this intended to be ongoing?

Comment on lines +28 to 35
<mat-error *ngIf="form.controls.name.errors?.required">
A project name is required.
</mat-error>
<mat-error *ngIf="form.controls.name.errors?.uniqueSlug">
A project with a name similar to “{{
form.controls.name.errors!.uniqueSlug.value
}}” already exists.
</mat-error>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<mat-error *ngIf="form.controls.name.errors?.required">
A project name is required.
</mat-error>
<mat-error *ngIf="form.controls.name.errors?.uniqueSlug">
A project with a name similar to “{{
form.controls.name.errors!.uniqueSlug.value
}}” already exists.
</mat-error>
@if (form.controls.name.errors?.required) {
<mat-error> A project name is required. </mat-error>
}
@if (form.controls.name.errors?.uniqueSlug) {
<mat-error>
A project with a name similar to “{{
form.controls.name.errors!.uniqueSlug.value
}}” already exists.
</mat-error>
}

Comment on lines +14 to +24
<mat-list-option
*ngFor="let t4cModel of t4cModelService.t4cModels$ | async"
[value]="t4cModel.id"
>
<div mat-line>TeamForCapella model '{{ t4cModel.name }}'</div>
<div mat-line>
repository '{{ t4cModel.repository.name }}', instance '{{
t4cModel.repository.instance.name
}}'
</div>
</mat-list-option>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<mat-list-option
*ngFor="let t4cModel of t4cModelService.t4cModels$ | async"
[value]="t4cModel.id"
>
<div mat-line>TeamForCapella model '{{ t4cModel.name }}'</div>
<div mat-line>
repository '{{ t4cModel.repository.name }}', instance '{{
t4cModel.repository.instance.name
}}'
</div>
</mat-list-option>
@for (
t4cModel of t4cModelService.t4cModels$ | async;
track t4cModel.id
) {
<mat-list-option [value]="t4cModel.id">
<div mat-line>TeamForCapella model '{{ t4cModel.name }}'</div>
<div mat-line>
repository '{{ t4cModel.repository.name }}', instance '{{
t4cModel.repository.instance.name
}}'
</div>
</mat-list-option>
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't actually tested this, just saw a few ngfors that could probably be easily swapped to the new format as well

@dominik003
Copy link
Collaborator Author

dominik003 commented Mar 28, 2024

So the changes I'm seeing make sense, though there are a number of logic changes I haven't been able to personally test. I'm kind of wondering what the criteria for updating is, since you left some of the old logic controls in components you updated. Is this intended to be ongoing?

So there were a few cases that I wasn't quite sure how we should handle. If I remember correctly, they were:

  • using *ngIf for mat-error and for certain, smaller span elements, as I wasn't quite sure if we want to use the new control flow there or if the normal inline approach is better
  • using *ngFor for mat-options as I wasn't sure and didn't have time to check if using the new control flow works as expected

I think I'll use your comments to think again about whether it makes sense to apply the new control flow to all conditions and loops, or whether we should perhaps exclude some scenarios in the next planning to keep it simple. You are also welcome to use this PR or issue #1373 to provide your input on how we should use the new control flow. I think it is very important that we all align on this topic and ultimately include it in our development documentation.

And finally, this PR only serves as a starting point for the new control flow. This is mainly because this PR already contains a large number of changes and I didn't want to make it too big. Also, applying these changes takes some time, as the new control flow has a lot of room for major simplifications, especially since we can now use else if and else properly.

@MoritzWeber0
Copy link
Member

MoritzWeber0 commented Apr 5, 2024

I really appreciate the effort you spent on the changes, but some components are really hard to test. Testing all of the modified changes will require a significant effort.

This week we've introduced storybook in the Capella Collaboration Manager. Storybook offers a nice UI and with the Chromatic integration stories will automatically scanned for visual changes. Due to mocking, the effort to implement stories low.

Therefore, we should have a story defined for the hard-to-test (e.g., authentication, logout) components to avoid some unintentional changes.

This commit applies the new Angular control flow introduced with
Angular 17 to several components. This includes some simplifications
that can now be applied more easily. In addition, some css classes
have been converted to tailwind and unused css files have been removed.
Last but not least, some line breaks and indentations in the components
have been adjusted to improve readability.
@dominik003 dominik003 force-pushed the refactor-apply-new-angular-control-flow branch from 93515ca to 49b3e3c Compare April 8, 2024 07:28
Copy link

sonarcloud bot commented Apr 8, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@MoritzWeber0
Copy link
Member

Due to the amount of conflicts, I'll close the PR. Instead, we'll iteratively switch to the new format, together with the introduction of stories for the the affected components.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants