Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## edge #21291 +/- ##
===========================================
- Coverage 57.27% 27.50% -29.78%
===========================================
Files 3991 3995 +4
Lines 327285 328010 +725
Branches 46564 46507 -57
===========================================
- Hits 187463 90211 -97252
- Misses 139603 237780 +98177
+ Partials 219 19 -200
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
sfoster1
left a comment
sfoster1
left a comment
There was a problem hiding this comment.
Looks good overall, but don't have time to give a more detailed review. Do you think you're going to put the reason field in here? Should this be the "access control" page rather than the "login" page?
There was a problem hiding this comment.
Functionally LGTM, just various nitpicks.
Visually, I think there are a couple design mismatches. If those are intentional for prototyping reasons, I'm happy for this to be merged and we can come back to them later, but can we leave // todo comments for them?
| onSuccess: () => { | ||
| navigate(-1) | ||
| }, |
There was a problem hiding this comment.
Will navigate(-1) restore the state inside whatever page the user was on previously?
Like, suppose:
- The user is on
/deck-configuration - They do some work
- Their session times out and the "locked" overlay appears
- They tap the "locked" overlay, which navigates to this
/loginpage - They log in successfully, we run this
navigate(-1), and return them to the/deck-configurationpage - But now it's as if they're visiting
/deck-configurationfor the first time. So all their work is gone? I think?
I haven't tested this, and the react-router-dom docs do say that -1 is "often used to close modals." But I don't see how that can work unless react-router-dom is doing something magical.
There was a problem hiding this comment.
using -1 isn't bad (since the component is touchscreen only and the likelihood of any issues occurring is quite low.), however, opentrons repo use it for the wizardflow basically.
so using a specific path would be easy for others.
| data-testid={ | ||
| showPassword ? 'login-password-hide' : 'login-password-show' | ||
| } |
There was a problem hiding this comment.
Nit: I don't think you need a data-testid here. The <Btn>'s aria-label and role="button" should already be enough for the tests to be able to find it without a data-testid.
There was a problem hiding this comment.
yes, please do not use data-testid.
data-testid should be treated as a last resort, so in principle it must not be used for unit tests. it’s only allowed as an exception for e2e.
| useEffect(() => { | ||
| if (!showKeyboard) return | ||
| const id = window.setTimeout(() => { | ||
| keyboardRef.current?.setInput(fieldValue) | ||
| }, 0) | ||
| return () => { | ||
| window.clearTimeout(id) | ||
| } | ||
| }, [showKeyboard, fieldValue, step]) |
There was a problem hiding this comment.
What's this timeout stuff for?
| useEffect(() => { | ||
| setShowPassword(false) | ||
| if (step === 'password') { | ||
| inputRef.current?.focus() |
There was a problem hiding this comment.
Nit: It'd probably be cleaner to use autoFocus instead.
| navigate(-1) | ||
| }, | ||
| onError: message => { | ||
| makeSnackbar(message) |
There was a problem hiding this comment.
The designs don't use snackbars for these errors—they have dedicated error text for "wrong password," "locked out," etc. Is this intentional?
I'm totally fine with using snackbars if it makes it easier to get the demo up and running, just making sure it's intentional.
| <h4 className={styles.field_label}> | ||
| {step === 'username' ? 'Username' : 'Password'} | ||
| </h4> |
There was a problem hiding this comment.
Are we intentionally not using StyledText for text styling?
This is a genuine question. I really don't know what the correct way of doing this is, these days.
There was a problem hiding this comment.
Generally speaking, using header text is good but this is an Electron app so it’s more important to align with the codebase’s implementation patterns than with general web best practices.
recommend you using StyledText and the design team uses oddStyle for text.
There was a problem hiding this comment.
i thought we should not use StyledText anymore?
There was a problem hiding this comment.
Ideally, yes but unfortunately we don't have a strategy on that change and we are not ready to change our implementation style. (+ the design team has registered desktopStyle and oddStyle on Figma)
We need to discuss whether to create a new CSS Modules–based component for the text, or to use the header text approach you suggested.
I think this transition will be expensive since for the first one we have to replace LegacyStyledText with StyledText first(some components use width/height directly so unfortunately a simple replace won't work) and for the second one, that will increase the review cost significantly.
| <h4 className={styles.field_label}> | ||
| {step === 'username' ? 'Username' : 'Password'} | ||
| </h4> |
There was a problem hiding this comment.
Nit: Can we have i18n strings for all of this copy, unless you're intentionally deferring that to another PR?
| onFocus={() => { | ||
| setShowKeyboard(true) | ||
| }} | ||
| rightElement={ |
There was a problem hiding this comment.
Putting the show/hide icon inside the text field doesn't match the designs—it should be outside.
There was a problem hiding this comment.
i know. but that is how we do it in other places in the code. i left a comment for mel
| const next = e.target.value | ||
| if (step === 'username') { | ||
| setUsername(next) | ||
| } else { | ||
| setPassword(next) | ||
| } |
There was a problem hiding this comment.
As opentrons' implementation style , when a handler grows beyond two lines, it’s preferable to extract it into a dedicated function like handleChange. The main reason is to keep the JSX easy to scan and read.
| step === 'password' ? ( | ||
| <Btn | ||
| type="button" | ||
| aria-label={showPassword ? 'Hide password' : 'Show password'} |
There was a problem hiding this comment.
Probably the existing component uses a conditional statement for aria-label but changing aria-label conditionally isn’t inherently wrong, but in this case it’s not ideal.
The current inputfield allows us use aria-*, so the following would be better.
aria-label="Toggle password visibility"
aria-pressed={showPassword}
| margin: 0 auto; | ||
| margin-top: 4.625rem; |
There was a problem hiding this comment.
can you avoid using margin for a new component?
the design team uses Figma auto layout and it doesn't use margin.
| @@ -0,0 +1,25 @@ | |||
| .nav_container { | |||
| height: 7.75rem; | |||
There was a problem hiding this comment.
i guess this would be for ChildNavigation
if so, please align the styling with app/src/organisms/ODD/CameraSettings/CameraControls/cameracontrols.module.css
There was a problem hiding this comment.
recommend you creating a mock instead of ApiHostProvider since Storybook is to check the look.
when our Storybook breaks, it’s often because we’re not using mocks and are instead wiring it up with the same providers and other app-level code that’s used in the real application.
There was a problem hiding this comment.
the fileName should be useOAuth2PasswordLogin.test.ts.
if you need to use .tsx instead of .ts, probably you will need to check the custom hook.
| const detail = data?.errors?.[0]?.detail | ||
| if (detail != null && detail !== '') return detail | ||
| if (error.response?.status != null) { | ||
| return `Login failed (${error.response.status})` |
There was a problem hiding this comment.
can you update the test for Login?
Co-authored-by: Koji Kanao <baxin1919@gmail.com>
Co-authored-by: Koji Kanao <baxin1919@gmail.com>
SyntaxColoring
left a comment
SyntaxColoring
left a comment
There was a problem hiding this comment.
Just one more thing I noticed while wiring this up to the LoggedOutOverlay.
| <ChildNavigation | ||
| header="Login" | ||
| buttonText={step === 'username' ? 'next' : 'confirm'} | ||
| buttonIsDisabled={primaryDisabled} | ||
| secondaryButtonProps={{ | ||
| buttonText: 'cancel', | ||
| buttonType: 'tertiaryLowLight', | ||
| onClick: handleCancel, | ||
| }} | ||
| onClickButton={handleNext} | ||
| /> |
There was a problem hiding this comment.
Another thing for matching the designs: there should be a back arrow up here when the user is on the password step, to return to the username step.
| <Controller | ||
| key={activeFieldName} | ||
| control={control} | ||
| name={activeFieldName} | ||
| render={({ field }) => ( |
There was a problem hiding this comment.
For my edification, what does Controller do and when should we use it?
4 participants
Overview
closes https://opentrons.atlassian.net/browse/EXEC-2163.
Adds ODD Login with a two-step flow (username → password)
Test Plan and Hands on Testing
[ ] ODD Login: enter username → password → confirm; success navigates back; bad credentials show snackbar (with dev-backend-flex or auth-server on 33950 + proxy on 31950).
[ ] Storybook: ODD → Pages → Login renders without provider errors.
Changelog
http://{{hostname}}:6060/?path=/story/odd-pages-login--defaultReview requests
am i missing anything?
Risk assessment
low. new logic.