Conversation
11f8ae8 to
1e55856
Compare
|
I like the way of having the measures in an out-of-view menu. (This delays confronting trainees with all available measures.) Also the placement seems like a good fit. Some thoughts for now:
A thought for the future:
|
|
I mostly agree with @fekoch, but additionally I would consider the following things
|
I was thinking about this too. I temporarily added a toast when a measure is executed and aborted, but this feels very spammy. I personally think we don't need a generic feedback, because most measures will trigger some behavior -- we could e.g. enforce that a measure always has at least one property which triggers UI, like a manual confirmation, which would make this obsolete.
Discussion question: should the user get feedback on the delay / should they know how long they have to wait? |
56ad713 to
a6d57b9
Compare
935d5a8 to
163596c
Compare
163596c to
dee842e
Compare
JohannesPotzi
left a comment
There was a problem hiding this comment.
mini initial review. More to come.
Signed-off-by: Julius Makowski <[email protected]>
Signed-off-by: Julius Makowski <[email protected]>
Signed-off-by: Julius Makowski <[email protected]>
Signed-off-by: Julius Makowski <[email protected]>
Signed-off-by: Julius Makowski <[email protected]>
Signed-off-by: Julius Makowski <[email protected]>
Signed-off-by: Julius Makowski <[email protected]>
Signed-off-by: Julius Makowski <[email protected]>
Signed-off-by: Julius Makowski <[email protected]>
Signed-off-by: Julius Makowski <[email protected]>
Signed-off-by: Julius Makowski <[email protected]>
Signed-off-by: Julius Makowski <[email protected]>
Signed-off-by: Julius Makowski <[email protected]>
Signed-off-by: Julius Makowski <[email protected]>
Signed-off-by: Julius Makowski <[email protected]>
Signed-off-by: Julius Makowski <[email protected]>
Signed-off-by: Julius Makowski <[email protected]>
Signed-off-by: Julius Makowski <[email protected]>
Signed-off-by: Julius Makowski <[email protected]>
fekoch
left a comment
There was a problem hiding this comment.
partial review for the shared directory
| function sendAlarmGroupVehicle( | ||
| draftState: WritableDraft<ExerciseState>, | ||
| vehicleParameters: VehicleParameters, | ||
| vehicleParameters: Immutable<VehicleParameters>, |
There was a problem hiding this comment.
Can you make VehicleParameters always immutable instead?
There was a problem hiding this comment.
I did not touch this in this PR. I don't know where this comes from.
There was a problem hiding this comment.
The change is already there on dev-next, just clean it up when merging
Signed-off-by: Julius Makowski <[email protected]>
Signed-off-by: Julius Makowski <[email protected]>
Signed-off-by: Julius Makowski <[email protected]>
Signed-off-by: Julius Makowski <[email protected]>
75fb15b to
86cc780
Compare
| } | ||
| ); | ||
| break; | ||
| case 'drawingInstance': |
There was a problem hiding this comment.
Why do we not have to also do the [Drawing] Add drawing action?
If the frontend independently proposes that, but not the addLogEntry actions, this seems very inconsitent to me
There was a problem hiding this comment.
The problem is that we want the drawings to be displayed before the measure is finally executed -- such that the user who is creating the drawing can see it before confirming/declining if there are more properties after the drawing, and also potential other participants and trainers can see this while it's being done.
While we could implement this for all other measure properties (i.e. creating an EOC log entry) as well, I found that drawings were more of an exception to the rule. Happy to discuss this though.
I had previously implemented for all drawings to be removed again and re-created by the Add Measure reducer, but this did not make any sense since it just added one more action and the state is exactly the same afterwards with both approaches.
There was a problem hiding this comment.
Robert and I talked a bit: One (albeit a bit more involved) way would be, to track draftDrawings as a observable inside the drawing service and, after injecting the service into the drawing feature manager, combine it's observable with the selectVisibleDrawings()-observable from the store.
That way, we could prevent prematurely sending actions about unfinished drawings.
But this could also be treated as future work.
Signed-off-by: Julius Makowski <[email protected]>
Signed-off-by: Julius Makowski <[email protected]>
f24e817 to
ed22bbe
Compare
|
Global measures don't work in fullscreen. |
Signed-off-by: Julius Makowski <[email protected]>
Summary
Closes #1350.
This PR aims to implement (global) measures by
MeasureProperty,MeasureTemplateandMeasureAlso
PR Checklist
Please make sure to fulfil the following conditions before marking this PR ready for review:
git commit -s), I certify that I have read and adhere to the terms of the Developer Certificate of Origin 1.1 and license the code in this Pull Request under this projects license (LICENSE-README.md).