Improve frontend code comments and refactor modules/app

rohanpurohit commented 3 years ago

This PR includes:

Issue #390:

cross-checked replies and comments from sudhir and updated wherever necessary, mostly additional info.
remove isVentilatorOn useState, ventilating selector is used
remove landingLabel useState

Issue #331:

remove useEffect calling initParameterUpdate,currently called from StartPauseVentilation button's OnClick handler.

Issue #361:

Updated the StartPauseVentilation button's OnClick handler to handle these 3 scenarios comment
Scenario 1 & 2 work as described
Scenario 3 (current behavior after this PR):
1. Ventilation is inactive
2. Firmware disconnects
3. User presses "Starts Ventilation" button
4. The button is not disabled, the button label is still Start ventilation
5. Firmware reconnects, ventilation is not started

Misc Refactor/feature addition:

added dispatchParameterRequest method for better code-readability.
added a separate button for LandingPage to simplify code and to avoid UseStates.
Added modal popup on pressing Pause Ventilation when there are unsaved changes in alarm limits
Added a ModalContent component that has discard or Keep Previous Values content of alarm modal.
ModalContent is reused in AlarmsPage, SideBarSlideRoute, ToolBar
Mute button is disabled on connection loss to firmware
Added TODO's from some of Sudhir's comments
remove eslint-disable-next-line react-hooks/exhaustive-deps

rohanpurohit commented 3 years ago

@ethanjli observations from testing on my end:

On disconnecting firmware, the start button now shows Connecting, but on plugging it back in, could you describe what do we want? currently, it toggles ventilation, which I suppose is undesirable. let me know!

rohanpurohit commented 3 years ago

@ethanjli This PR is ready for testing/code review. I have updated the PR description with the latest changes, in terms of code quality:

I did add a useState for opening the modalPopup on Pause Ventilation and a separate OnConfirm handler for the same.
Other than that a different useState for disabling the buttons as the StartPauseVentilation button is not really disabled now other than when it says Connecting... following the scenarios here #361 comment.

ethanjli commented 3 years ago

Thanks, I will test and review. Were you able to identify the source of the behavior you reported about the backend's "firmware disconnected" alarm not causing the frontend to show it as an active alarm (with a red border)?

rohanpurohit commented 3 years ago

Thanks, I will test and review. Were you able to identify the source of the behavior you reported about the backend's "firmware disconnected" alarm not causing the frontend to show it as an active alarm (with a red border)?

sorry! I forgot about that, moreover so because we have disabled the mute button on firmware disconnection, but I realize we have to find the cause for that regression. I will do that now!

rohanpurohit commented 3 years ago

@ethanjli the behavior actually makes sense, so the mute button sends a request when it's clicked to change the active state of AlarmMute message and that certainly won't happen when the firmware is disconnected, and the red border and audio for alarms looks at the getAlarmMuteActive selector which is AlarmMute message active field, so whatever was the state before firmware was disconnected remains, that is if mute was active before disconnection, there won't be a border/audio and vice-versa. for the backend disconnection we look at just AlarmMuteRequest active therefore we can toggle the border and audio.

ethanjli commented 3 years ago

I see. From your explanation, I must have misunderstood what the behavior was. I had thought that you meant that when the firmware got disconnected, backend created an active alarm but that never triggered the red border in the frontend (even when alarms weren't muted). Instead, it looks like the behavior is as follows:

When alarms aren't muted, the "firmware disconnected" alarm behaves just like any other alarm, which is what we expect.
When alarms are muted and then the "firmware disconnected" alarm is generated, the frontend don't cancel the active alarm mute. So this isn't a regression, it's just missing functionality which needs to be implemented as part of #372. As you say, the behavior makes sense for how we've currently implemented it.

This observation is useful for informing how we should implement #372, though I haven't thought about that enough to propose any concrete approaches.

ethanjli commented 3 years ago

Observations from testing against #361: everything works as expected, except when ventilation is inactive and the firmware disconnects - the comment I made at https://github.com/pez-globo/pufferfish-software/issues/361#issuecomment-863733123 only partially specified the desirable behavior, by describing a less desirable behavior. Perhaps what we should have here is that when the backend or firmware are disconnected in standby mode, then the "Start Ventilation" is disabled and says either "Loading...", "Connecting...", or something else. Let's discuss to see if this behavior would make the most sense for usability and safety, or if we should have some other behavior.

I'm also wondering if it might make more sense to say "Loading..." instead of "Connecting...", or even to have the button label say "Starting Ventilation..." or "Pausing Ventilation..." in their corresponding situations instead of "Connecting...".

Additional observations:

When I'm on the Alarm Limits screen during ventilation and press "Pause Ventilation", I stay on the Alarm Limits screen. When I'm on the Settings screen during ventilation and press "Pause Ventilation", I stay on the Settings screen. By contrast, when I'm on either screen during standby and press "Start Ventilation", I am taken to the dashboard. For consistency, maybe it'd make sense that pressing "Pause Ventilation" should take me back to the Quickstart screen?
~In EventAlerts.tsx I see code to disable the Alarm Mute button when the firmware is disconnected. But when I actually disconnect the firmware, I don't see this button getting changing or being disabled.~ I'm not able to reproduce this, I'll look out to see if it happens again

rohanpurohit commented 3 years ago

This project is licensed under Apache License v2.0 for any software, and Solderpad Hardware License v2.1 for any hardware - do you agree that your contributions to this project will be under these licenses, too? Yes.
Were any of these contributions also part of work you did for an employer or a client? No.
Does this work include, or is it based on, any third-party work which you did not create? No.

pez-globo / pufferfish-software