-
Notifications
You must be signed in to change notification settings - Fork 155
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
feat: Add plugin slot for login page #1354
base: master
Are you sure you want to change the base?
Conversation
Thanks for the pull request, @xitij2000! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
e19d19d
to
8dfabed
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1354 +/- ##
==========================================
- Coverage 87.69% 87.67% -0.02%
==========================================
Files 124 125 +1
Lines 2299 2304 +5
Branches 646 645 -1
==========================================
+ Hits 2016 2020 +4
- Misses 274 275 +1
Partials 9 9 ☔ View full report in Codecov by Sentry. |
8dfabed
to
e0b12b2
Compare
const renderHook = (hookCallback) => { | ||
const result = { | ||
current: null, | ||
}; | ||
|
||
const Component = () => { | ||
const val = hookCallback(); | ||
React.useEffect(() => { | ||
result.current = val; | ||
}); | ||
return null; | ||
}; | ||
|
||
render(<Component />); | ||
return { result }; | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to remove @testing-library/react-hooks
due to package conflicts, so I've replaced it with this simplified hook testing code. In later versions this hook is part of the react testing library so we can pull it from there and remove this.
const LoginPageSlot = ({ | ||
institutionLogin, | ||
handleInstitutionLogin, | ||
}) => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how valid this is. The original component needs these values, however they are not included in plugin props since they seem pretty specific to the implementation of the login page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basing on many similar code examples here, I think it's valid, @xitij2000?
clearThirdPartyAuthContextErrorMessage, | ||
}, | ||
)(Logistration); | ||
export default Logistration; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the changes here are to move from old style redux code to more modern react hooks based code.
@@ -111,7 +116,7 @@ const Logistration = (props) => { | |||
{!institutionLogin && ( | |||
<h3 className="mb-4.5">{formatMessage(messages['logistration.sign.in'])}</h3> | |||
)} | |||
<LoginPage institutionLogin={institutionLogin} handleInstitutionLogin={handleInstitutionLogin} /> | |||
<LoginPageSlot institutionLogin={institutionLogin} handleInstitutionLogin={handleInstitutionLogin} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using the LoginPageSlot in two places but perhaps these should be two slots that both render the LoginPage by default?
src/login/LoginPage.jsx
Outdated
shouldBackupState: false, | ||
showResetPasswordSuccessBanner: false, | ||
submitState: DEFAULT_STATE, | ||
thirdPartyAuthApiStatus: PENDING_STATE, | ||
thirdPartyAuthContext: { | ||
currentProvider: null, | ||
errorMessage: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes to this file are to move to hooks from the older pattern of mapStateToProps etc.
src/login/LoginPage.jsx
Outdated
const mapStateToProps = state => ({ | ||
thirdPartyAuthContext: thirdPartyAuthContextSelector(state), | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't we fetching thirdPartyAuthContext
with useSelector
above? If so, can't we get rid of mapStateToProps
completely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started moving to the new hooks-based approach but ran out of time. I've now fully migrated.
const LoginPageSlot = ({ | ||
institutionLogin, | ||
handleInstitutionLogin, | ||
}) => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basing on many similar code examples here, I think it's valid, @xitij2000?
e0b12b2
to
df9184b
Compare
50d2066
to
c999936
Compare
This change adds a plugin slot for the login page allowing it to be customised. Since there was a dependency conflict between frontend-plugin-framework and the react-hooks testing package, the react-hooks testing package has been removed and a replaced with a simple mechanism for testing hooks. Since this touched the Login Page those have also been refactored to move away from redux connect.
c999936
to
f1a8f7b
Compare
@xitij2000 Do we need to backport this PR to Sumac? |
Description
This change adds a plugin slot for the login page allowing it to be customised.
Since there was a dependency conflict between frontend-plugin-framework and the react-hooks testing package, the react-hooks testing package has been removed and a replaced with a simple mechanism for testing hooks.
Since this touched the Login Page those have also been refactored to move away from redux connect.
How Has This Been Tested?
Testing with a simple env.config.jsx, using automated tests and manual testing of login
Screenshots/sandbox (optional):
Include a link to the sandbox for design changes or screenshot for before and after. Remove this section if its not applicable.
Merge Checklist
Post-merge Checklist