Refactor frontend modules/display and screensaver

[rohanpurohit]

This PR includes:

• Turning bannerType into an Enum
• Renaming the Data type internal to the MultiStepWizard to InternalState
• Renaming setValue and setValueActual to ValueDraft and ValueCurrent respectively
• using mcu_pb interface Range instead of number[]
• getting rid of functions in MultiStepWizard accessing store directly as they weren't react functional components, now selectors and hooks are used instead.
• MultiStepWizard, ModalPopup, DiscardAlarmLimitsContent and EventAlerts are now moved to modals folder.
• ToogleValue is now moved to controllers as its an interactive component
• DashBoardPage is cleaned up as we only have HFNCMainView for now.
• ValuesPage and corresponding info components are removed (reason: they are deprecated, and restrict us from re-structuring other components like ValueModal and ValueDisplay)
• alarmLimitsSelector is updated to return Range instead of number[]

[rohanpurohit]

Need to work more on this! not ready for testing

[rohanpurohit]

I feel like I'm changing way too many files and should stop before I make too many changes as it gets harder to review! @ethanjli It looks like I have resolved most of the GitHub review comments but I haven't resolved them in this PR so that you can refer to them while reviewing! but there's a chance I might have missed something.

In terms of TO-DO's in issue #399, things that are unresolved are: Renaming ValueDisplay and ValueSelectorDisplay to something more descriptive/specific.

As it turns out only one component currently is using ValueDisplay which is ValueController, which is re-used across a lot of active components in our frontend, which are:

1. QuickStartPage, these are the two ValueClickers for FiO2 and Flow on the Quickstart screen, turns out they were indeed using ValueController and not just ValueClicker, it was just imported as ValueClicker which is misleading! fixed that in https://github.com/pez-globo/pufferfish-software/pull/413/commits/dcc0ab9548f52606afe725143af74a3d77d6021a and some other changes to quickstart page(next point)
   • changed the switch block for ventilation mode returning JSX to a Map, the defaultContent Element that's returned when Ventilation mode is null or the Map returns undefined is currently Never because mode is selected as HFNC before we reach the QuickStart Page, thus what I did is in the ValueClicker I'm setting the onClick callback to null, we need to fix these when we have different modes.
   • The reason for that is related to #419 and this TODO(overall really weird structure of the setValue method) I'll probably get rid of that when I'm fixing #419
2. the second one is in DisplayTab these are brightness, hour, day, month etc

So the point is initially we had decided if you recall that we'll get rid of ValueDisplay and stick to ValueSelectorDisplay because we thought the latter was used widely, now as it turns out ValueDisplay is used far more frequently in our active code

• so I thought about removing ValueSelectorDisplay instead (reasons were: it's just a simple react fragment with value) which I tried here https://github.com/pez-globo/pufferfish-software/pull/413/commits/c9e3d09cadb46312c3ed45e23c2f127a27e03550 Problems i faced with that approach were: we lose all the redraw fixes we did in #407 as the whole Info component would render instead of just the value part (not exactly like the issue we had, its just one component that changes redraws continuously) therefore I reverted that now!
• and the argument for having ValueDisplay around is 1. has a different look 2. not all values that its used for has a selector currently and cant be replaced by ValueSelectorDisplay entirely. So, In conclusion, I'm leaning towards keeping them both! and if so maybe we can rename them? or not?

Testing: I have done mild testing with the changes, I'll do more tomorrow!

[rohanpurohit]

Here's my review. Everything looks good to me.

Yes, it makes sense to save the issue of names for ValueDisplay and ValueSelectorDisplay for a future PR. Maybe you can split this off from #399 into a separate Github issue?

Yes I can do that!

Do you have any insight into whether we actually need to import ValueClicker in some components and ValueController in others, or whether we can import only one of them for all components? I forget if we had already come to a conclusion on this. Maybe the answer will change after #419 is addressed.

I believe we haven't discussed this! So basically ValueClicker is just the two buttons for Increment/Decrement (see here) and we play around with the visual for Value, so it does make sense to keep ValueClicker separate (and it anyway has complicated logic related to how it handles the value that's passed) whereas ValueController is a simple component with ValueDisplay and ValueClicker together. if you take a look at AlarmsPage we use the ValueClicker component as it is, and the visual for value is defined locally, instead of using ValueDisplay or ValueController to replace both! see (AlarmsPage L246) So having the Value separated from ValueClicker allows us to do that!

As for the visual appearance, maybe ValueDisplay should be modified so that its style can be controlled via props.

I guess we can definitely do that, and sort of unify how Value and ValueClickers are used together across the frontend.

[ethanjli]

So basically ValueClicker is just the two buttons for Increment/Decrement (see here) and we play around with the visual for Value, so it does make sense to keep ValueClicker separate (and it anyway has complicated logic related to how it handles the value that's passed) whereas ValueController is a simple component with ValueDisplay and ValueClicker together. if you take a look at AlarmsPage we use the ValueClicker component as it is, and the visual for value is defined locally, instead of using ValueDisplay or ValueController to replace both! see (AlarmsPage L246) So having the Value separated from ValueClicker allows us to do that!

Ah ok, that really helps! Let's modify the code comments to include this nuance, including the relationship between the two components and where it would be better to use a ValueController instead of directly using a ValueClicker. Let's also make sure that we only use ValueClicker in places where there isn't a corresponding ValueDisplay to go along with it (e.g. we can use it in the alarms page). Maybe it would make sense to rename ValueController to ValueSpinner, in the sense of https://en.wikipedia.org/wiki/Spinner_(computing) - I think "Spinner" is more descriptive than "Controller" about what ValueController actually looks like or behaves like.

In a future PR, we could consider passing a component into the props of ValueController which could be a ValueDisplay or any custom component to render a single value (such as what is used on AlarmsPage); in that case, then it should be possible to use ValueController on AlarmsPage, and maybe then the only file in our app which needs to import ValueSpinner will be ValueController.

[ethanjli]

There's a regression in the "HFNC Control" tab of the breadcrumbs modal in the alarm limits displayed for FiO2 and Flow: they should be displaying the actual alarm limits (i.e. +/- 2 from the current parameter value). However, on this PR they are just shown as [0 - 100]. The previous implementation was also buggy: after changing a parameter setting and committing it, opening the breadcrumbs modal again should reload the alarm limits to the new values which the firmware or simulator backend automatically set based on the parameter value, but instead the alarm limits never change after the frontend initially loads. Since the previous implementation was also buggy, I think we can fix these problems in a future PR, if you create a separate Github issue to track this problem.

I haven't noticed any other problems in this PR when trying to do "normal" user actions on the frontend.

[rohanpurohit]

So basically ValueClicker is just the two buttons for Increment/Decrement (see here) and we play around with the visual for Value, so it does make sense to keep ValueClicker separate (and it anyway has complicated logic related to how it handles the value that's passed) whereas ValueController is a simple component with ValueDisplay and ValueClicker together. if you take a look at AlarmsPage we use the ValueClicker component as it is, and the visual for value is defined locally, instead of using ValueDisplay or ValueController to replace both! see (AlarmsPage L246) So having the Value separated from ValueClicker allows us to do that!

Ah ok, that really helps! Let's modify the code comments to include this nuance, including the relationship between the two components and where it would be better to use a ValueController instead of directly using a ValueClicker. Let's also make sure that we only use ValueClicker in places where there isn't a corresponding ValueDisplay to go along with it (e.g. we can use it in the alarms page). Maybe it would make sense to rename ValueController to ValueSpinner, in the sense of https://en.wikipedia.org/wiki/Spinner_(computing) - I think "Spinner" is more descriptive than "Controller" about what ValueController actually looks like or behaves like.

renamed ValueController to ValueSpinner and added code comments

In a future PR, we could consider passing a component into the props of ValueController which could be a ValueDisplay or any custom component to render a single value (such as what is used on AlarmsPage); in that case, then it should be possible to use ValueController on AlarmsPage, and maybe then the only file in our app which needs to import ValueSpinner will be ValueController.

that's a great idea! yes, we should.

There's a regression in the "HFNC Control" tab of the breadcrumbs modal in the alarm limits displayed for FiO2 and Flow: they should be displaying the actual alarm limits (i.e. +/- 2 from the current parameter value). However, on this PR they are just shown as [0 - 100]. The previous implementation was also buggy: after changing a parameter setting and committing it, opening the breadcrumbs modal again should reload the alarm limits to the new values which the firmware or simulator backend automatically set based on the parameter value, but instead the alarm limits never change after the frontend initially loads. Since the previous implementation was also buggy, I think we can fix these problems in a future PR, if you create a separate Github issue to track this problem.

Oh yeah, that is a bug, I think it should be fixed now in the latest commit, if it's still buggy I'll open a separate issue.

[rohanpurohit]

1. 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.
2. Were any of these contributions also part of work you did for an employer or a client? No.
3. Does this work include, or is it based on, any third-party work which you did not create? No.

[rohanpurohit]

There's a regression in the "HFNC Control" tab of the breadcrumbs modal in the alarm limits displayed for FiO2 and Flow: they should be displaying the actual alarm limits (i.e. +/- 2 from the current parameter value). However, on this PR they are just shown as [0 - 100]. The previous implementation was also buggy: after changing a parameter setting and committing it, opening the breadcrumbs modal again should reload the alarm limits to the new values which the firmware or simulator backend automatically set based on the parameter value, but instead the alarm limits never change after the frontend initially loads. Since the previous implementation was also buggy, I think we can fix these problems in a future PR, if you create a separate Github issue to track this problem.

I believe it's completely fixed now on https://github.com/pez-globo/pufferfish-software/pull/413/commits/4dedf8dca06a3c639b5ca5e5236eb9785082cb43, if you could just confirm that then I won't create a separate issue for this! @ethanjli

[ethanjli]

Yup, it looks like it's been solved!