Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor password field validation and restyle #1081

Merged
merged 5 commits into from
Oct 19, 2022
Merged

Conversation

lukaw3d
Copy link
Member

@lukaw3d lukaw3d commented Oct 12, 2022

I tried to make validation part of the input field, so that onSubmit doesn't need to re-check validity of every input field. But then error-specific-styling doesn't work contentProps={{ border: props.error ? 'bottom' : false }}.

Before After
localhost_3000_open-wallet_private-key (2) localhost_3000_open-wallet_private-key

@lukaw3d lukaw3d requested review from buberdds and csillag October 12, 2022 23:42
@github-actions
Copy link

github-actions bot commented Oct 12, 2022

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ EDITORCONFIG editorconfig-checker 5 0 0.01s
✅ REPOSITORY checkov yes no 15.85s
✅ REPOSITORY git_diff yes no 0.0s
✅ TSX eslint 4 0 0 6.86s

See errors details in artifact MegaLinter reports on CI Job page
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

@codecov
Copy link

codecov bot commented Oct 12, 2022

Codecov Report

Merging #1081 (821c8a8) into master (7114b6f) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1081      +/-   ##
==========================================
+ Coverage   88.69%   88.75%   +0.05%     
==========================================
  Files         102      102              
  Lines        1778     1778              
  Branches      412      411       -1     
==========================================
+ Hits         1577     1578       +1     
+ Misses        201      200       -1     
Flag Coverage Δ
cypress 60.57% <100.00%> (ø)
jest 78.86% <100.00%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/app/components/PasswordField/index.tsx 87.50% <100.00%> (-1.39%) ⬇️
...s/OpenWalletPage/Features/FromPrivateKey/index.tsx 100.00% <100.00%> (ø)
src/app/lib/key.ts 100.00% <0.00%> (+6.25%) ⬆️

@lukaw3d lukaw3d force-pushed the lw/refactor-password branch from fb0447f to 1f0727f Compare October 13, 2022 03:29
onChange: (event: React.ChangeEvent<HTMLInputElement>) => void
error: string | false
value?: string
onChange?: (event: React.ChangeEvent<HTMLInputElement>) => void
Copy link
Contributor

@buberdds buberdds Oct 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like these type changes do not belong to this PR. I don't see any use cases for optional value and onChange or TFormValue here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new recommended usage is with name and validate, but value, onChange, error still come useful. And keeping them is consistent with grommet's FormField.

I added some tests to validate both uses

@lukaw3d lukaw3d force-pushed the lw/refactor-password branch from 464f1bf to 1a21cf4 Compare October 18, 2022 22:38
@lukaw3d lukaw3d force-pushed the lw/refactor-password branch from 1a21cf4 to 821c8a8 Compare October 19, 2022 08:14
@lukaw3d lukaw3d merged commit c344d1f into master Oct 19, 2022
@lukaw3d lukaw3d deleted the lw/refactor-password branch October 19, 2022 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants