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

fix: Use redux hooks in Authn login page #1188

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

attiyaIshaque
Copy link
Contributor

@attiyaIshaque attiyaIshaque commented Mar 4, 2024

Description

This pull request intends to enhance the loginPage and Logistration components by adopting the React-Redux hooks useSelector and useDispatch in place of the conventional mapStateToProps and mapDispatchToProps functions. This update is aimed at improving maintainability.

This work is done in an effort to make the code in Authn consistent. Currently, we are using a mix of redux hooks and mapStateToProps and mapDispatchToProps functions.

JIRA

VAN-1810

How Has This Been Tested?

Locally

Copy link

codecov bot commented Mar 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.22%. Comparing base (8be350e) to head (fade402).
Report is 6 commits behind head on master.

❗ Current head fade402 differs from pull request most recent head 35a55ac. Consider uploading reports for the commit 35a55ac to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1188   +/-   ##
=======================================
  Coverage   84.22%   84.22%           
=======================================
  Files         126      126           
  Lines        2396     2384   -12     
  Branches      680      677    -3     
=======================================
- Hits         2018     2008   -10     
+ Misses        358      356    -2     
  Partials       20       20           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@arbrandes arbrandes self-requested a review March 4, 2024 16:52
@arbrandes
Copy link

I know this is still just a draft, but before making it official please include in the description the rationale behind this change. I'd like to review the PR once it's ready. Thanks!

@attiyaIshaque attiyaIshaque marked this pull request as ready for review March 5, 2024 10:46
@attiyaIshaque attiyaIshaque requested a review from a team as a code owner March 5, 2024 10:46
@zainab-amir
Copy link
Contributor

@arbrandes this PR still needs an internal review. Please review once we are done with our team's review on this. Thank you

Copy link
Contributor

@mubbsharanwar mubbsharanwar left a comment

Choose a reason for hiding this comment

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

Look's good to me.

src/logistration/Logistration.test.jsx Outdated Show resolved Hide resolved
@syedsajjadkazmii
Copy link
Contributor

Hi, @arbrandes!
This PR is ready for review.

@zainab-amir
Copy link
Contributor

@arbrandes do we need to wait for your review on this?

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.

None yet

5 participants