Closed Sudhir-dev closed 3 years ago
Thanks for your comprehensive responses to our many questions, @Sudhir-dev ! Your replies to questions we raised are good enough for knowledge transfer regarding the interfaces of the frontend functions, so I won't do an additional round of review on the code comments, because we don't have time to do this right now on Blackstraw's timeline. For anything in your replies which you haven't already incorporated into the code comments, Rohan and I can go back over the comments and do this later. I will do the formal Github approval now, and it'd be great if you could press the "Squash and merge" button. Thanks so much for all the work you've done for this project - if it weren't for you, our user interface literally would not exist!
Here is a list of comments which need further discussion (but we can merge this PR in before they are fully resolved):
Below is a list of proposed changes in the frontend based on our discussions, which we can convert into issues later (@Sudhir-dev is tagged on relevant items), and which @rohanpurohit and I can work on:
In general:
modules
folder into something which is consistent (and easy to navigate for someone new to the project, and for us in 2 years) across the whole app. This might also involve reorganizing the redux files into feature-based slices as mentioned in #342, and as illustrated at https://redux.js.org/style-guide/style-guide#structure-files-as-feature-folders-with-single-file-logic . Since this is such a big task, maybe we could spread this out over multiple PRs, each of which consolidates certain functionalities into a single folder, or moves a group of related files into a folder; we could start with just the React components, step-by-step, and later reorganize the Redux files, step-by-step. Each PR for this reorganization should include an index.ts
file in each folder it touches, and each index.ts
file should describe what belongs in that folder, what it provides, etc. For now, we probably don't need to worry about adding exports into the index.ts
files. This reorganization should only be done after we've made sure all replies on this PR are included in the code comments.modules/app:
staticStart
and having a different react component for the Start
button in the landing page vs. the "Start/Stop Ventilation" button in the other pages, to simplify the code: https://github.com/pez-globo/pufferfish-software/pull/359#discussion_r637011812isVentilatorOn
, which is dead code: https://github.com/pez-globo/pufferfish-software/pull/359#discussion_r637016143 , https://github.com/pez-globo/pufferfish-software/pull/359#discussion_r637020070 . This may also remove the need for a landingLabel
useState
? That was discussed in https://github.com/pez-globo/pufferfish-software/pull/359#discussion_r634079544Start
button in the landing page?) refactoring the way values get committed to ...Request
s when the Start Ventilation
button is pressed: https://github.com/pez-globo/pufferfish-software/pull/359#discussion_r637060011modules/controllers:
disableAlarmButton
for removal as deprecated code (for now we can just comment it out): https://github.com/pez-globo/pufferfish-software/pull/359#discussion_r637224968AlarmModal
to have one component which returns just the content inside the modal, and one component which returns the content wrapped inside an AlarmModal popop (which might not even be needed?)? https://github.com/pez-globo/pufferfish-software/pull/359#discussion_r637230277Range
object (interface with lower
and upper
) instead of a variable-length array for alarm limits? https://github.com/pez-globo/pufferfish-software/pull/359#discussion_r637232889useEffect
in ValueClicker
? https://github.com/pez-globo/pufferfish-software/pull/359#discussion_r637247627ValueModal
and ControlInfo
for deprecation, and marking HFNCGraphView
as dead code which will be rehabilitated in the future (i.e. once we get trends plotting, which may be rather far in the future): https://github.com/pez-globo/pufferfish-software/pull/359#discussion_r637255464 and https://github.com/pez-globo/pufferfish-software/pull/359#discussion_r637256035 and https://github.com/pez-globo/pufferfish-software/pull/359#discussion_r637257945modules/dashboard:
updateModalStatus
function and replacing it with setOpen
in ValueInfo
and LargeValueInfo
: https://github.com/pez-globo/pufferfish-software/pull/359#discussion_r637272133 and https://github.com/pez-globo/pufferfish-software/pull/359#discussion_r634128239ValueInfoProps
for deprecation in ValueInfo
and LargeValueInfo
: https://github.com/pez-globo/pufferfish-software/pull/359#discussion_r637390919isLive
from ValueInfo
and LargeValueInfo
: https://github.com/pez-globo/pufferfish-software/pull/359#discussion_r637391594ValueInfo
and LargeValueInfo
by unifying shared logic, while still providing different components to be used for different purposes, or parameterizing a single component or making its style size-responsive (e.g. for narrower grid boxes, wider grid boxes, one- vs. two-column-wide grid goxes, huge grid boxes with big font), and moving them to the modules/views
folder: https://github.com/pez-globo/pufferfish-software/pull/359#discussion_r637393473 and https://github.com/pez-globo/pufferfish-software/pull/359#discussion_r637393365ValueInfo
component with a "main container" and two "sub-containers" into a "ControlValuesGroup" component which can specify the dimensions and relative layout of its constituent "ControlValuesDisplay" components (and maybe ControlValuesDisplay
maybe should be renamed to ValueInfo
?): https://github.com/pez-globo/pufferfish-software/pull/359#discussion_r637392436modules/displays:
modules/displays
, and moving files in and out as needed: https://github.com/pez-globo/pufferfish-software/pull/359#discussion_r634146314bannerType
into an Enum
: https://github.com/pez-globo/pufferfish-software/pull/359#discussion_r634159464Data
type internal to the MultiStepWizard to something more descriptive: https://github.com/pez-globo/pufferfish-software/pull/359#discussion_r634161558committedSetting
with setValueActual
, if possible: https://github.com/pez-globo/pufferfish-software/pull/359#discussion_r637402148toggleBetween
field from an array of an unnamed object type to an array of ToggleSetting
objects, where the ToggleSetting
is defined as an interface with label
and value
fields: https://github.com/pez-globo/pufferfish-software/pull/359#discussion_r637403155ValueDisplay
and ValueSelectorDisplay
to something more descriptive/specific: https://github.com/pez-globo/pufferfish-software/pull/359#discussion_r637404018 and https://github.com/pez-globo/pufferfish-software/pull/359#discussion_r637404432modules/screensaver:
Thank you @ethanjli and @rohanpurohit for your support. I am thankful to have work with you. I will continue to support on anything where I can be of help.
Thank you @ethanjli and @rohanpurohit for your support. I am thankful to have work with you. I will continue to support on anything where I can be of help.
Thanks @Sudhir-dev for your kind words. Great experience working with you and thanks for all the help and contribution to this project.
This PR consists