fix: various adjustments to chevron positioning (#1238) #1338
Conversation
Grodien
requested review from
faebum,
mghilardelli,
phirzel,
rawi-coding and
smallTrogdor
as code owners
| // THEN | ||
| expect(animationData1, isNotNull); | ||
| expect(animationData1!.startOffset, 0.0); | ||
| expect(animationData1.endOffset, 108.0); |
There was a problem hiding this comment.
die Zahlen sind ein bisschen magisch -
ich denke ihnen einen Namen zu geben, würde helfen den Test besser zu verstehen, weil aktuell weiss man nicht so ganz was erwartet wird
There was a problem hiding this comment.
Hmm... absolut nicht sicher was du für ein Namen hier erwartest. "ExpectedAnimationData1StartOffset = 0.0" würde absolut kein value adden. Ich habe die Titel der Tests noch angepasst, damit sie sprechender sind.
...t/app/test/pages/journey/train_journey/widgets/table/config/chevron_animation_data_test.dart
Show resolved
Hide resolved
...t/app/test/pages/journey/train_journey/widgets/table/config/chevron_animation_data_test.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
cool mit den Tests, ich denke die magischen Zahlen noch einen Namen zu geben oder einen besseren Titel in den Testbeschreibungen hilft zu verstehen, was getestet werden soll.
There was a problem hiding this comment.
Titel ist verbessert, variablen Namen sehe ich nicht wie das helfen soll :/
| import 'package:sfera/component.dart'; | ||
| import 'package:sfera/src/model/journey/order_priority.dart'; | ||
|
|
||
| class GroupedJourneyPoint extends JourneyPoint { |
There was a problem hiding this comment.
Einerseits verstehe ich den Need, andererseits frage ich mich schon, ob das SFERA Modul wirklich wissen muss, dass bestimmte JourneyPoints gruppiert werden müssen (eigentlich nur wegen Anforderungen an die Anzeige)...
Frage mich das auch schon länger bei den BaliseLevelCrossingGroupings...
Also wir leaken halt recht viel Anzeige Details hier ins Sfera modul..
There was a problem hiding this comment.
Ja du hast wohl mit allen Punkten Recht, jedoch ist das nicht wirklich einfach weg zu bekommen und der Benefit Rechtfertigt ein Riesen Refactoring aktuell meiner Meinung nach nicht. Hatten die Diskussion ja glaub schonmal.
Z.b. base_data_extension liegt auch im sfera drin, was eigentlich das grouping fürs UI macht.
| other is LevelCrossing && | ||
| runtimeType == other.runtimeType && | ||
| order == other.order && | ||
| _originalOrder == other._originalOrder && |
There was a problem hiding this comment.
hm, wenn originalOrder nicht gesetzt ist, wird die order im == berücksichtigt, wenn es gesetzt ist, dann nicht ... das könnte zu ekligen Bugs führen.. lieber dann auch order immer berücksichtigen..
There was a problem hiding this comment.
Original order ist immer gesetzt (wird im consturctor auf order gesetzt, wenn es nicht mitgegeben wird).
Order wird im == nicht berücksichtigt, damit das equals funktioniert auch wenn die order für die anzeige auf die order der balise gesetzt wird. Ist also intended dass order im vergleich fehlt.
| @override | ||
| bool operator ==(Object other) => | ||
| identical(this, other) || | ||
| other is BaliseLevelCrossingGroup && |
There was a problem hiding this comment.
BaliseLevelCrossingGroup ersetzen
| required super.order, | ||
| required super.kilometre, | ||
| }) : super(type: Datatype.levelCrossing); | ||
| int? originalOrder, |
There was a problem hiding this comment.
wozu wird originalOrder eigentlich benötigt?
There was a problem hiding this comment.
Wie im anderen Kommentar: für das equals (damit es in der Gruppe gefunden wird).
2 participants
No description provided.