Open modal popup if sidebar is accessed from alarms screen

rohanpurohit commented 3 years ago

This PR adds functionality to open confirmation dialog on clicking the pufferfish icon that opens the sidebar if there are unsaved changes on the alarm limits screen.

rohanpurohit commented 3 years ago

Reasons for this approach:

settings button on sidebar is dynamically added by routes in Navigation, and to accommodate changes for confirmation dialog led to a sort of static approach instead of the dynamic nature currently, also breaks quite a few things, that was something i tried sometime back, i could try again!
if we also need a confirmation dialog on pressing the home button on the sidebar then that would mean duplicating the same code in SidebarClickable.

Thus, the current approach felt more feasible, we could maybe discuss whether on confirmation from modal popup, would we still want to toggle the sidebar? so that the user is not forced to press again.

ethanjli commented 3 years ago

Right now the user flow is unintuitive because pressing the pufferfish icon when there are unsaved changes doesn't actually open the sidebar. I suspect that if we toggle the sidebar upon confirming to discard unsaved changes, usability will be better. However, I'm not entirely sure whether usability will be acceptable with that change. Let's merge in this PR after making the sidebar be shown after pressing the Confirm button to discard unsaved changes, and then if we get feedback about the usability later, then we can figure out what it would take to be able to show the confirmation dialog only when pressing buttons in the sidebar.

My intuition for that deeper solution is that we'd need to find a way to add a custom onClick handler callback function to each sidebar button, and a custom callback for determining whether to proceed to the route represented by the button; then the onClick handler would set something to show the modal, and the confirm button of the modal would set whether to proceed to the route. I haven't looked at the architecture of the routing or sidebar systems enough to know whether this is feasible or easy or even possible, but I suspect it would require a deeper reworking of those systems - which should only be done after we make sure all of Sudhir's replies to our questions about the code comments have been integrated into the frontend.

One interesting consequence of the approach taken by this PR is that it's not possible to lock the screen from the sidebar while there are unsaved changes to alarm limits - in that condition, the screen can only be locked with the hardware button. This is probably fine, however.

ethanjli commented 3 years ago

This works fine in light testing. For records-keeping:

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

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

Open modal popup if sidebar is accessed from alarms screen #381