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

IPA Lias #880

Merged
merged 70 commits into from
Jun 21, 2024
Merged

IPA Lias #880

merged 70 commits into from
Jun 21, 2024

Conversation

lkleisa
Copy link
Collaborator

@lkleisa lkleisa commented Apr 4, 2024

  • New Switch Overview | Network
  • New Diagram which displays the alignments in bubbles connected with arrows
  • Diagram changes when filter in header changes
  • Diagram contains Nodes for Objectives and KeyResults

Diagram Information

  • Objective-Search in Header does only search for bubbles which are objectives
  • On bubble click, the sidepanel appears
  • It's possible to edit an object or add a checkin over the diagram sidepanel
  • The diagram does a full reload, if one object changes (too big effort to change one specific bubble svg)
  • The diagram does not affect the url, so it's not possible to send a link to the diagram
  • The library does to placement of the bubbles by it's own
  • On side reload or change of one object, the diagram does a reload and the placement changes
  • The objective contains an icon for the state, the keyresult only the background color (Phillip Leimgruber)

@clean-coder
Copy link
Collaborator

Bin zufällig auf deinen PR gestossen. Ich wollte fragen, ob du für getAlignmentsByFilters() noch einen Test schreiben wirst. Die Method ist ziemlich gross und auch komplex. Vielleicht könnte man da noch versuchen, den Code etwas "zu vereinfachen". Aber es ist nur eine Frage bzw. Vorschlag. Kannst mich auch gerne ignorieren.

@lkleisa
Copy link
Collaborator Author

lkleisa commented Apr 5, 2024

Hello, danke für deinen Input. Keine Angst, werde die Tests nächste Woche für den gesamten neuen Code schreiben. Die Methode getAlignmentsByFilters() wird da sicher ausführlich getestet. Jedoch sehe ich gerade nicht, was ich noch mehr vereinfachen könnte. Evt. ab Zeile 186 könnte man auslagern. Hast du einen Vorschlag?

@clean-coder
Copy link
Collaborator

Melde dich einfach, wenn du jUnit Tests für getAlignmentsByFilters() geschrieben hast. Ich würde bei dann mal den Versuch machen, ob ich (mit wenig) Aufwand den Code (bei mir lokal) noch lesbarer machen könnte. Falls ich erfolgreich sein sollte, kann ich dir das zeigen. Und ev. kannst da auch was davon verwenden. Ich würde versuchen, einige Stellen im Code in zusätzliche Methoden (mir beschreibendem Namen) zu extrahieren.

Ich find was du gemacht hast wirklich gut. Die Methode ist aber ziemlich lange und auch nicht trivial. Und ich weiss nicht (aus eigener Erfahrung), wie gut du diesen Code in ein paar Monaten noch verstehst. ... weil es halt nicht trivial ist.

Also melde dich. Wenn du nicht willst, melde dich nicht. Ich schaue was ich anpassen würde ... zeige es dir ... wenns nicht gefällt, dann ignoriere meinen Input einfach. Es ist nur ein Angebot .

@lkleisa
Copy link
Collaborator Author

lkleisa commented Apr 5, 2024

Danke für dein Angebot. Du kannst gerne versuchen, den Code schöner zu machen, wenn du das willst.

Wie der Branch und der PR sagen, bin ich an der IPA, der individuellen Praxisarbeit, welche über meine Lehrnote entscheiden wird. Deshalb ist es mir nicht erlaubt, Code, welcher jemand anderes geschrieben hat, für die Umsetzung zu verwenden. Deshalb kann ich mir deinen Code auch nicht ansehen, da dies gegen die Vorgaben ist.
Falls ich nach der IPA vor dem Merging in den Main noch Interesse habe, würde ich mich bei dir melden

@lkleisa lkleisa changed the title Ipa lias IPA Lias Apr 9, 2024
@lkleisa lkleisa self-assigned this Apr 9, 2024
@lkleisa lkleisa force-pushed the IPA-Lias branch 8 times, most recently from aa86fb4 to 58ff7c7 Compare April 12, 2024 06:15
@lkleisa lkleisa force-pushed the IPA-Lias branch 2 times, most recently from 4412739 to c7dc444 Compare April 29, 2024 06:04
@lkleisa lkleisa force-pushed the IPA-Lias branch 5 times, most recently from ff22425 to 00068ee Compare May 1, 2024 13:05
@lkleisa lkleisa force-pushed the Probe-IPA-Lias branch 2 times, most recently from 3c31ca6 to ec5bb77 Compare May 23, 2024 06:52
Copy link
Collaborator

@janikEndtner janikEndtner left a comment

Choose a reason for hiding this comment

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

Hi Lias
Habe mir das Frontend deiner IPA auch noch angeschaut :-) Ich werde in den nächsten Tagen noch etwas testen, aber sieht gut aus!

frontend/src/app/diagram/diagram.component.ts Outdated Show resolved Hide resolved
frontend/src/app/diagram/diagram.component.ts Outdated Show resolved Hide resolved
frontend/src/app/diagram/diagram.component.ts Outdated Show resolved Hide resolved
frontend/src/app/diagram/diagram.component.ts Outdated Show resolved Hide resolved
frontend/src/app/diagram/diagram.component.ts Outdated Show resolved Hide resolved
frontend/src/app/diagram/diagram.component.ts Outdated Show resolved Hide resolved
frontend/src/app/diagram/diagram.component.ts Show resolved Hide resolved
frontend/src/app/shared/services/refresh-data.service.ts Outdated Show resolved Hide resolved
@lkleisa lkleisa force-pushed the IPA-Lias branch 3 times, most recently from c311771 to 625f71c Compare June 21, 2024 11:45
@lkleisa lkleisa merged commit 254422a into Probe-IPA-Lias Jun 21, 2024
5 checks passed
@Miguel7373 Miguel7373 deleted the IPA-Lias branch October 31, 2024 08:24
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.

4 participants