capella-collab-manager icon indicating copy to clipboard operation
capella-collab-manager copied to clipboard

refactor: Apply new Angular control flow

Open dominik003 opened this issue 10 months ago • 4 comments

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.

dominik003 avatar Mar 26 '24 16:03 dominik003

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 76.94%. Comparing base (b04fb2e) to head (49b3e3c). Report is 269 commits behind head on main.

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           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Mar 26 '24 16:03 codecov[bot]

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.

dominik003 avatar Mar 28 '24 12:03 dominik003

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.

MoritzWeber0 avatar Apr 05 '24 11:04 MoritzWeber0

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.

MoritzWeber0 avatar May 27 '24 13:05 MoritzWeber0