[Verify] Update new-architecture input fields on base screens to pass through all textfield props #461
Conversation
manojleaton
requested review from
surajeaton,
daileytj and
JeffGreiner-eaton
as code owners
github-actions
bot
added
the
brightlayer-ui
|
Visit the preview URL for this PR (updated for commit 7501b30): https://blui-react-login--pr461-4302-adding-textfi-dox9hmyf.web.app (expires Fri, 15 Sep 2023 10:35:14 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 1e9064d2e35ed37fe01e053587ea5f209719a043 |
| @@ -99,6 +100,7 @@ export const ChangePasswordDialogBase: React.FC<ChangePasswordDialogProps> = (pr | |||
| id="current-password" | |||
| label={currentPasswordLabel} | |||
| value={currentPassword} | |||
| {...currentPasswordTextFieldProps} | |||
There was a problem hiding this comment.
Do we need to merge OnPasswordChange here like https://github.com/etn-ccis/blui-react-workflows/blob/dev/login-workflow/src/screens/AccountDetailsScreen/AccountDetailsScreen.tsx#L142.
Same comment for other screens
|
And ensure you update docs too, including the readmes and jsdocs |
| @@ -84,13 +86,15 @@ export const SetPassword: React.FC<React.PropsWithChildren<SetPasswordProps>> = | |||
| inputRef={passwordRef} | |||
| label={newPasswordLabel} | |||
| value={passwordInput} | |||
| error={shouldValidatePassword && !isValidPassword()} | |||
| sx={TextFieldStyles(theme)} | |||
| {...passwordTextFieldProps} | |||
There was a problem hiding this comment.
Any particular reason the passwordTextFieldProps is spreaded here as opposed to spreaded towards the end of this props list? I'd like to hear your reasoning.
There was a problem hiding this comment.
Same for the other spreads you added in this pull request
There was a problem hiding this comment.
To override the defaults which we set in the base screen components, spreaded the TextFieldProps towards the end of the props list.
There was a problem hiding this comment.
I do not understand. I see that you still have onChange, onKeyUp and onBlur under the {...passwordTextFieldProps}, so this is not the end of the props list of PasswordTextField?
There was a problem hiding this comment.
Just had a discussion with Joe in Office hours regarding this, so we need to merge events with TextFieldProps like we did for WorkFlowActions.
login-workflow/src/components/ChangePasswordDialog/ChangePasswordDialogBase.tsx
Outdated
Show resolved
Hide resolved
| onChange={(evt: ChangeEvent<HTMLInputElement>): void => onPassChange(evt.target.value)} | ||
| error={shouldValidatePassword && !isValidPassword()} | ||
| sx={TextFieldStyles(theme)} | ||
| {...passwordTextFieldProps} |
There was a problem hiding this comment.
just so I know, why do we have both of these? just for theme aware items? plus additional sx styles?
sx={TextFieldStyles(theme)}
{...passwordTextFieldProps}
There was a problem hiding this comment.
Raised a PR for removed passing theme object to functions to generate styles. sx prop is for adding styles.
Spreading passwordTextFieldProps to pass text field specific props. Like variant, onChange and etc.
|
Just putting this here so we all know... Confused when running tests locally with yarn test and running the one test file. locally when running yarn test I see failed test here... 
and notice the test error description doesn't match the test I found that can't find the role input. The actual test with description "should show success screen, when okay button is clicked" has no findAllByRole in it but the previous test does, very strange 
when I run only the test file itself the test passes and also passes in circleCI. the findAllByRole query is searching the entire DOM and this is known as a slow running query because of that. We will need to keep an eye on these types of queries and be more direct by using getByRole |
Fixes #BLUI-4302 .
Changes proposed in this Pull Request:
To Test:
Any specific feedback you are looking for?