Skip to content
This repository has been archived by the owner on Sep 18, 2023. It is now read-only.

refactor(views/SignIn): split component and hooks #114

Merged
merged 4 commits into from
Aug 8, 2023

Conversation

sbolel
Copy link
Contributor

@sbolel sbolel commented Jul 26, 2023

Summary

This PR focuses on improving the accessibility and best practices of the existing sign-in view. It splits the SignIn view into multiple files, and pulls out hooks into a custom hook instead of implementing them inside the component itself. It also includes changes related to the file structure, moving all sign-in related files into a single directory.

The main changes are:

  • The form input fields now have associated labels that are correctly linked using the "for" attribute for better accessibility.
  • The usage of react-hook-form library for managing form state and validation is now consistent throughout the entire form.
  • Error messages are now tied to the corresponding input field with aria-describedby attribute, improving accessibility.
  • Created a new form component FormControlInput that is a reusable form input with react-hook-form controls.
  • Created a new helper component PasswordVisibilityToggle to manage the show/hide password functionality, enhancing code reusability.
  • Extracted custom styled components into a separate module to keep the main component code cleaner.
  • The form's submit handler now checks the form validity before proceeding with the login logic.
  • The sign-in hook now uses the Auth module from AWS Amplify for handling the federated sign in.
  • Introduced Jest tests for the useSignIn hook, increasing the code's reliability.

Other:

  • moved SignInGraphic into SignIn.components.tsx

Related Issues

How to test

  • yarn start - start the application and log in to verify that the sign in page behaves as expected.

Coverage:

-------------------------------------|---------|----------|---------|---------|---------------------------
File                                 | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
-------------------------------------|---------|----------|---------|---------|---------------------------
 src/components                      |         |          |         |         |
  PasswordVisibilityToggle.tsx       |     100 |      100 |     100 |     100 |
 src/components/forms                |     100 |    81.81 |     100 |     100 |
  InputFormControl.tsx               |     100 |       80 |     100 |     100 | 31-52
  SubmitButton.tsx                   |     100 |      100 |     100 |     100 |
 src/views/SignIn                    |    97.5 |      100 |    92.3 |    98.5 |
  SignIn.components.tsx              |      92 |      100 |    87.5 |   92.85 | 61
  SignIn.tsx                         |     100 |    57.14 |     100 |     100 | 133,149
  useSignIn.ts                       |   75.75 |      100 |   66.66 |      75 | 82-92

@sbolel sbolel added the refactor This issue or PR is a refactor of existing code label Jul 26, 2023
@sbolel sbolel self-assigned this Jul 26, 2023
@sbolel sbolel added this to the UI MVP milestone Jul 26, 2023
Copy link
Contributor

@qtpeters qtpeters left a comment

Choose a reason for hiding this comment

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

I'm having a hard time correctly configuring this application. I have a file named: .env.development.local I have been using to configure it, however I'm still getting:

image

because UserPoolId is not undefined. I tried having that config file at the root of the application, then I put exports on the variables in the file and imported the variables into the environment. Neither worked. Do me a monster favor and put detailed configuration instructions into the README or the PR instructions (README is better because it will stay there for future reference) and I'll review it again.

@sbolel sbolel force-pushed the refactor/views/SignIn-split-up branch from cd98a48 to 030c570 Compare August 7, 2023 22:04
@sbolel
Copy link
Contributor Author

sbolel commented Aug 7, 2023

@qtpeters all you need to do is

cp .env.test .env.development.local

and then enter in the right values for the dev environment. I sent you these values on Slack.

DerekStrickland
DerekStrickland previously approved these changes Aug 7, 2023
Copy link
Contributor

@DerekStrickland DerekStrickland left a comment

Choose a reason for hiding this comment

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

All tests passing

@sbolel sbolel force-pushed the refactor/views/SignIn-split-up branch from c7f61c9 to 1719b7e Compare August 8, 2023 01:35
@sbolel sbolel requested a review from qtpeters August 8, 2023 11:40
@sbolel sbolel force-pushed the refactor/views/SignIn-split-up branch from a7caba0 to 45d663e Compare August 8, 2023 11:44
qtpeters
qtpeters previously approved these changes Aug 8, 2023
Copy link
Contributor

@qtpeters qtpeters left a comment

Choose a reason for hiding this comment

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

Testing it worked. LGTM

@sbolel sbolel force-pushed the refactor/views/SignIn-split-up branch from bcc3a48 to ea2e4ac Compare August 8, 2023 12:11
@sbolel sbolel requested a review from qtpeters August 8, 2023 12:13
Copy link
Contributor

@qtpeters qtpeters left a comment

Choose a reason for hiding this comment

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

👍

@sbolel sbolel merged commit 6045294 into main Aug 8, 2023
1 check passed
@sbolel sbolel deleted the refactor/views/SignIn-split-up branch August 8, 2023 12:14
@github-actions
Copy link

🎉 This PR is included in version 1.4.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@github-actions github-actions bot added the released This has been released label Aug 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
refactor This issue or PR is a refactor of existing code released This has been released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Autocomplete and ARIA Attributes to Form Fields in SignIn View
3 participants