refactor(app): Desktop implementation for SelectRecoveryOption #15691
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mjhuff
reviewed
Jul 17, 2024
Comment on lines
+5
to
+9
| // note: this typescript stuff is so that e.currentTarget.value in the ChangeEventHandler | ||
| // is deduced to a union of the values of the options passed to the radiogroup rather than | ||
| // just string |
StandIn can have children, VisibleContainer gets some customization so it works properly in flow cases. StandIn can now be used in TwoColumn. TwoColumn has its fixed width popped out into constants and now takes advantage of VisibleContainer to work slightly better.
We have a radio button group in the desktop app, but it is still using CSS modules and kind of awful to use. Make a quick wrapper that allows customization via the ways that we currently like to customize things.
This was a single component that had both the step lister thing and a forced two-column layout, which isn't really useful. Make FailedStepNextStep its own component and use it in the TwoCol.
This is a structural component that will render as two columns on desktop and as one column (not displaying the right-column component) on ODD. This is a pattern that is used in semantically-equivalent cross platform layouts throughout error recovery, so let's implement it as an intervention modal component here.
The container was just the ContentWrapper and was implicitly only single column; make versions for two columns and optional numbers of columns as well, since those are layouts that are used throughout ER. Also handle footer buttons, since those can be a little tricky (they should be inside the content, or can be anyway, if SingleColumn but definitely outside and spanning if TwoColumn). This requires renaming the component instantiations in a whole bunch of places, which is annoying, but such is life.
This implements the desktop styling for SelectRecoveryOption. Specifically, - On desktop this should be a twocolumn with failedstepnextstep, and on ODD this should be a onecolumn with just the buttons; that means use the fancy one or two columns wrapper - The radio buttons need desktop styles, so use the fancy RecoveryRadioGroup component Unfortunately, since we're rendering FailedStepNextStep we need access to the full analysis in tests which means we need to change the fixtures which means some other components needed to handle having the fixtures changed.
sfoster1
force-pushed
the
fix-desktop-select-recovery
branch
from
3566602 to
e63d05a
Compare
mjhuff
approved these changes
Jul 18, 2024
| @@ -0,0 +1 @@ | |||
| export const TWO_COLUMN_ELEMENT_MIN_WIDTH = '17.1875rem' as const | |||
There was a problem hiding this comment.
Just curious - aree we getting this number from the ER Figma or is there some InterventionModal master design file that more formally specify this?
There was a problem hiding this comment.
That is from figma I think, and if not from a f2f convo about the appropriate size https://www.figma.com/design/8dMeu8MuPfXoORtOV6cACO/Feature%3A-Error-Recovery-August-Release?node-id=2523-42677&t=VW7DaWomoCsOTNti-4
| > | ||
| {t('choose_a_recovery_action')} | ||
| </StyledText> | ||
| {isOnDevice ? ( |
There was a problem hiding this comment.
we cant lose isOnDevice here?
There was a problem hiding this comment.
Yeah I suppose we could!
TamarZanzouri
approved these changes
Jul 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR got away from me a little bit (and I still have to test it - the sum of these two is why it's in draft). It's probably best to view it commit by commit.
Overall, the point is to implement the desktop styling for the SelectRecoveryOption component of error recovery, here: https://www.figma.com/design/8dMeu8MuPfXoORtOV6cACO/Feature%3A-Error-Recovery-August-Release?node-id=4829-78111&t=bTwOek0mZSA61ugm-4 . This is a little weird because it is exactly semantically equivalent to the ODD panel, but has a very different styling - two columns instead of one. It also uses the radio buttons that we use only in OT-2 tip length calibration method selection, which are old and use cssmodules and need a cleanup.
The approach here was to add some fundamental structure components to InterventionModal that will render two columns on desktop (as the TwoColumn structure, including min-size and wrap) and one column on the ODD at full width. Then, we can build on top of that in ErrorRecovery, with the big difference being that the ErrorRecovery component also folds in the standard ER footer (or will - already had to find/replace a bunch of stuff, going to move other components over to specifying their footers through the wrapper as we update their styles).
There were also some incidental refactors. By commit,
Todo