Conversation
…rprise-theming Aut 17 update auth mfe to support enterprise theming
There was a problem hiding this comment.
Pull request overview
This pull request implements enterprise theming changes for the authentication MFE, including a major refactoring from Redux connect to React hooks (useDispatch/useSelector), new enterprise branding layout components, and associated styling changes.
Changes:
- Refactored
Logistration.jsxandLoginPage.jsxfrom ReduxconnectHOC to React hooks for state management - Added new enterprise-themed hero layouts (SmallLayout, MediumLayout, LargeLayout) with support for enterprise branding
- Added enterprise theming SCSS styles and updated existing layout styles
- Extended data utilities with enterprise redirect URL functions
- Added
enterpriseBrandingsupport to the Redux store and selectors
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 24 comments.
Show a summary per file
| File | Description |
|---|---|
| src/sass/_base_component.scss | Added new auth-hero styling classes for enterprise theming, reformatted existing gradient styles |
| src/logistration/Logistration.test.jsx | Added React import, mocked plugin framework, updated config values from strings to booleans, changed test selectors |
| src/logistration/Logistration.jsx | Refactored from connect to hooks, added LoginComponentSlot plugin integration, added enterprise branding support |
| src/login/LoginPage.jsx | Refactored from connect to hooks, removed cohesion tracking, simplified tracking calls |
| src/data/utils/dataUtils.js | Added enterprise redirect URL utility functions and improved redirectWithDelay function |
| src/data/constants.js | Changed APP_NAME value from 'authn_mfe' to 'authn', added 'enterprise_customer' to AUTH_PARAMS |
| src/common-components/data/tests/reducer.test.js | Added enterpriseBranding field to default state test |
| src/common-components/data/reducers.js | Added enterpriseBranding to state, reformatted SUCCESS case |
| src/base-container/components/welcome-page-layout/messages.js | Added new message keys for enterprise hero text |
| src/base-container/components/welcome-page-layout/SmallLayout.jsx | Replaced progressive profiling layout with enterprise-themed hero layout |
| src/base-container/components/welcome-page-layout/MediumLayout.jsx | Replaced progressive profiling layout with enterprise-themed hero layout |
| src/base-container/components/welcome-page-layout/LargeLayout.jsx | Replaced progressive profiling layout with enterprise-themed hero layout |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .auth-hero-heading-line.text-accent-a { | ||
| color: #03C7E8 !important; // for "with edX" |
There was a problem hiding this comment.
CSS specificity: Using !important can make future style overrides difficult. Consider increasing selector specificity or restructuring the CSS to avoid the need for !important.
| .auth-hero-heading-line.text-accent-a { | |
| color: #03C7E8 !important; // for "with edX" | |
| .auth-hero-heading .auth-hero-heading-line.text-accent-a { | |
| color: #03C7E8; // for "with edX" |
src/logistration/Logistration.jsx
Outdated
| } from '../data/utils'; | ||
| import { LoginPage } from '../login'; | ||
| import { backupLoginForm } from '../login/data/actions'; | ||
| import LoginComponentSlot from '../plugin-slots/LoginComponentSlot'; |
There was a problem hiding this comment.
Missing module: The import LoginComponentSlot from '../plugin-slots/LoginComponentSlot' references a file that doesn't exist in the codebase. Based on the test mock at line 31-34 in Logistration.test.jsx, it appears this should be a plugin slot that wraps LoginPage. Either create the missing plugin slot file or import LoginPage directly.
| import LoginComponentSlot from '../plugin-slots/LoginComponentSlot'; | |
| import { LoginPage as LoginComponentSlot } from '../login'; |
| window.location = { hostname: getConfig().SITE_NAME, href: getConfig().BASE_URL }; | ||
|
|
||
| const props = { selectedPage: LOGIN_PAGE }; | ||
| render(reduxWrapper(<Logistration />)); |
There was a problem hiding this comment.
Double render: The component is rendered twice on consecutive lines (277 and 278), with the second render destructuring container. This means the test will be interacting with the wrong render. Remove the first render on line 277.
| render(reduxWrapper(<Logistration />)); |
| <div | ||
| className="auth-hero-message mt-3" | ||
| dangerouslySetInnerHTML={{ __html: enterpriseWelcomeHtml }} | ||
| /> |
There was a problem hiding this comment.
Security: Using dangerouslySetInnerHTML with enterpriseWelcomeHtml without sanitization can expose the application to XSS vulnerabilities. The HTML content comes from enterpriseBranding?.enterpriseBrandedWelcomeString or enterpriseBranding?.platformWelcomeString which appears to come from backend data. Ensure this content is properly sanitized on the backend, or use a sanitization library like DOMPurify on the frontend before rendering.
| case THIRD_PARTY_AUTH_CONTEXT.SUCCESS: { | ||
| return { | ||
| ...state, | ||
| fieldDescriptions: action.payload.fieldDescriptions?.fields, | ||
| optionalFields: action.payload.optionalFields, | ||
| thirdPartyAuthContext: { | ||
| ...action.payload.thirdPartyAuthContext, | ||
| enterpriseBranding: action.payload.thirdPartyAuthContext.enterpriseBranding || null, | ||
| }, | ||
| thirdPartyAuthApiStatus: COMPLETE_STATE, | ||
| }; |
There was a problem hiding this comment.
Potential data loss: The SUCCESS case previously set countriesCodesList: action.payload.countriesCodesList which is now removed. If this data is still needed elsewhere in the application, this change could break functionality. Verify that countriesCodesList is no longer needed or is handled elsewhere.
| case THIRD_PARTY_AUTH_CONTEXT.SUCCESS: { | ||
| return { | ||
| ...state, | ||
| fieldDescriptions: action.payload.fieldDescriptions?.fields, | ||
| optionalFields: action.payload.optionalFields, | ||
| thirdPartyAuthContext: { | ||
| ...action.payload.thirdPartyAuthContext, | ||
| enterpriseBranding: action.payload.thirdPartyAuthContext.enterpriseBranding || null, | ||
| }, | ||
| thirdPartyAuthApiStatus: COMPLETE_STATE, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Inconsistent indentation: The case statement and return block have inconsistent indentation. The case on line 32 should be indented at the same level as other case statements (lines 27, 45), and the return block should be consistently indented.
| case THIRD_PARTY_AUTH_CONTEXT.SUCCESS: { | |
| return { | |
| ...state, | |
| fieldDescriptions: action.payload.fieldDescriptions?.fields, | |
| optionalFields: action.payload.optionalFields, | |
| thirdPartyAuthContext: { | |
| ...action.payload.thirdPartyAuthContext, | |
| enterpriseBranding: action.payload.thirdPartyAuthContext.enterpriseBranding || null, | |
| }, | |
| thirdPartyAuthApiStatus: COMPLETE_STATE, | |
| }; | |
| } | |
| case THIRD_PARTY_AUTH_CONTEXT.SUCCESS: { | |
| return { | |
| ...state, | |
| fieldDescriptions: action.payload.fieldDescriptions?.fields, | |
| optionalFields: action.payload.optionalFields, | |
| thirdPartyAuthContext: { | |
| ...action.payload.thirdPartyAuthContext, | |
| enterpriseBranding: action.payload.thirdPartyAuthContext.enterpriseBranding || null, | |
| }, | |
| thirdPartyAuthApiStatus: COMPLETE_STATE, | |
| }; | |
| } |
| @@ -227,7 +239,10 @@ describe('Logistration', () => { | |||
|
|
|||
| const props = { selectedPage: LOGIN_PAGE }; | |||
| render(reduxWrapper(<Logistration {...props} />)); | |||
There was a problem hiding this comment.
Double render: The component is rendered twice on consecutive lines (241 and 242), with the second render destructuring container. This means the test will be interacting with the wrong render. Remove the first render on line 241.
| render(reduxWrapper(<Logistration {...props} />)); |
| {enterpriseWelcomeHtml && ( | ||
| <div | ||
| className="auth-hero-message mt-4" | ||
| dangerouslySetInnerHTML={{ __html: enterpriseWelcomeHtml }} | ||
| /> |
There was a problem hiding this comment.
Security: Using dangerouslySetInnerHTML with enterpriseWelcomeHtml without sanitization can expose the application to XSS vulnerabilities. The HTML content comes from enterpriseBranding?.enterpriseBrandedWelcomeString or enterpriseBranding?.platformWelcomeString which appears to come from backend data. Ensure this content is properly sanitized on the backend, or use a sanitization library like DOMPurify on the frontend before rendering.
| background: #ffffff; | ||
| border-radius: 6px; | ||
| padding: 0.75rem 1rem; | ||
| display: inline-flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| } | ||
|
|
||
| .auth-hero-enterprise-logo { | ||
| max-width: 145px; | ||
| max-height: 74px; | ||
| object-fit: contain; | ||
| } | ||
|
|
||
| /* yellow slanted line */ | ||
| .auth-hero-slash { | ||
| width: 0; // only height+border matters | ||
| height: 110px; // tune this to match logo/heading height | ||
| border-left: 6px solid #F0CC00; // Figma yellow | ||
| border-radius: 999px; | ||
| transform: skewX(-12deg); // gives that forward slash look | ||
| transform-origin: center; | ||
| } | ||
|
|
||
| /* heading text: "Start learning" + "with edX" */ | ||
| .auth-hero-heading { | ||
| display: flex; | ||
| flex-direction: column; | ||
| } | ||
|
|
||
| .auth-hero-heading-line { | ||
| font-family: 'Inter', sans-serif; | ||
| font-size: 60px; | ||
| font-weight: 700; | ||
| line-height: 60px; | ||
| letter-spacing: -1.2px; | ||
| color: #FFFFFF; // for "Start learning" | ||
| } | ||
|
|
||
| .auth-hero-heading-line.text-accent-a { | ||
| color: #03C7E8 !important; // for "with edX" | ||
| } | ||
|
|
||
| /* enterprise message aligned under heading, same left edge */ | ||
| .auth-hero-message { | ||
| max-width: 492px; | ||
| color: #fff; | ||
| font-family: 'Inter', sans-serif; | ||
| font-size: 22px; | ||
| font-weight: 400; | ||
| line-height: 36px; | ||
| } | ||
|
|
||
| .auth-hero-message p { | ||
| margin: 0; | ||
| color: inherit; | ||
| font-size: inherit; | ||
| line-height: inherit; | ||
| } | ||
|
|
||
| .auth-hero-message a { | ||
| color: #03c7e8; | ||
| text-decoration: underline; | ||
| } |
There was a problem hiding this comment.
Maintainability: Hardcoded color values (#F0CC00, #03C7E8, #ffffff, #03c7e8) are used throughout the SCSS. Consider using CSS custom properties (CSS variables) or SCSS variables that are already defined in the project (like var(--pgn-color-*)) for better maintainability and theming consistency.
|
|
||
| const LargeLayout = ({ fullName }) => { | ||
|
|
||
| const LargeLayout = ({ fullName = null }) => { |
There was a problem hiding this comment.
Unused parameter: The fullName parameter is defined with a default value but never used in the component. Either remove this unused parameter or use it if it's intended for future functionality.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 14 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| window.location = { hostname: getConfig().SITE_NAME, href: getConfig().BASE_URL }; | ||
|
|
||
| const props = { selectedPage: LOGIN_PAGE }; | ||
| render(reduxWrapper(<Logistration />)); |
There was a problem hiding this comment.
The component is being rendered twice here. Line 277 renders the component without capturing the result, then line 278 renders it again and captures the container. This is wasteful and could cause unexpected behavior in tests. Remove line 277 since line 278 already renders the component properly.
| render(reduxWrapper(<Logistration />)); |
| @@ -228,7 +241,6 @@ const LoginPage = (props) => { | |||
| success={loginResult.success} | |||
| redirectUrl={loginResult.redirectUrl} | |||
| finishAuthUrl={finishAuthUrl} | |||
There was a problem hiding this comment.
The RedirectLogistration component receives currentProvider but has a typo in its prop definition (currectProvider instead of currentProvider). While removing this prop from LoginPage appears intentional, ThirdPartyAuth component at line 304 still receives currentProvider. You should verify that removing currentProvider from RedirectLogistration doesn't break cohesion event tracking logic at line 34 of RedirectLogistration.jsx, which checks if (!currectProvider).
| finishAuthUrl={finishAuthUrl} | |
| finishAuthUrl={finishAuthUrl} | |
| currectProvider={currentProvider} |
| expect(sendTrackEvent).toHaveBeenCalledWith('edx.bi.institution_login_form.toggled', { category: 'user-engagement', app_name: APP_NAME }); | ||
| expect(sendPageEvent).toHaveBeenCalledWith('login_and_registration', 'institution_login', { app_name: APP_NAME }); |
There was a problem hiding this comment.
The tracking events in this test still expect the app_name parameter, but it was removed from the tracking calls in the updated code (lines 75, 77, 79 in Logistration.jsx). Update the test expectations to remove app_name from the expected parameters to match the actual implementation.
| expect(sendTrackEvent).toHaveBeenCalledWith('edx.bi.institution_login_form.toggled', { category: 'user-engagement', app_name: APP_NAME }); | |
| expect(sendPageEvent).toHaveBeenCalledWith('login_and_registration', 'institution_login', { app_name: APP_NAME }); | |
| expect(sendTrackEvent).toHaveBeenCalledWith('edx.bi.institution_login_form.toggled', { category: 'user-engagement' }); | |
| expect(sendPageEvent).toHaveBeenCalledWith('login_and_registration', 'institution_login', {}); |
| export const PASSWORD_RESET_CONFIRM = '/password_reset_confirm/:token/'; | ||
| export const PAGE_NOT_FOUND = '/notfound'; | ||
| export const ENTERPRISE_LOGIN_URL = '/enterprise/login'; | ||
| export const APP_NAME = 'authn'; |
There was a problem hiding this comment.
The APP_NAME constant was changed from 'authn_mfe' to 'authn', but this change may break analytics tracking and reporting that depend on the specific 'authn_mfe' identifier. Verify that all analytics dashboards, reports, and downstream systems are updated to handle 'authn' instead of 'authn_mfe', or consider maintaining backward compatibility.
| export const APP_NAME = 'authn'; | |
| export const APP_NAME = 'authn_mfe'; |
| <div className="auth-hero-slash" aria-hidden="true"> | ||
| <svg xmlns="http://www.w3.org/2000/svg" width="191" height="250" viewBox="0 0 191 250" fill="none" style={{ width: '100%', height: '100%' }}> | ||
| <line x1="69.8107" y1="33.833" x2="32.9503" y2="206.952" stroke="#F0CC00" strokeWidth="8" /> | ||
| </svg> | ||
| </div> |
There was a problem hiding this comment.
The auth-hero-slash div has specific styling (width: 0, height: 110px, border-left: 6px) but contains an inline SVG with viewBox and percentage-based dimensions that override the container. This creates confusion about which approach is being used. Either use the border-based approach (removing the SVG) or use the SVG approach (removing the border/width/height styles). The current implementation mixes both approaches inconsistently.
| <div className="auth-hero-slash" aria-hidden="true"> | |
| <svg xmlns="http://www.w3.org/2000/svg" width="191" height="250" viewBox="0 0 191 250" fill="none" style={{ width: '100%', height: '100%' }}> | |
| <line x1="69.8107" y1="33.833" x2="32.9503" y2="206.952" stroke="#F0CC00" strokeWidth="8" /> | |
| </svg> | |
| </div> | |
| <div className="auth-hero-slash" aria-hidden="true" /> |
| render(reduxWrapper(<Logistration {...props} />)); | ||
| fireEvent.click(screen.getByText('Institution/campus credentials')); | ||
| const { container } = render(reduxWrapper(<Logistration {...props} />)); |
There was a problem hiding this comment.
The component is being rendered twice here. Line 241 renders the component but doesn't capture the result, then line 242 renders it again. This is wasteful and could cause unexpected behavior in tests. Remove line 241 since line 242 already captures the container properly.
| <div | ||
| className="auth-hero-message mt-3" | ||
| dangerouslySetInnerHTML={{ __html: enterpriseWelcomeHtml }} | ||
| /> |
There was a problem hiding this comment.
Using dangerouslySetInnerHTML without sanitization is a security risk. The enterpriseWelcomeHtml contains HTML from enterpriseBrandedWelcomeString or platformWelcomeString which could potentially include malicious scripts. Consider sanitizing the HTML content using a library like DOMPurify before rendering it.
| font-family: 'Inter', sans-serif; | ||
| font-size: 60px; | ||
| font-weight: 700; | ||
| line-height: 60px; | ||
| letter-spacing: -1.2px; | ||
| color: #FFFFFF; // for "Start learning" | ||
| } | ||
|
|
||
| .auth-hero-heading-line.text-accent-a { | ||
| color: #03C7E8 !important; // for "with edX" | ||
| } | ||
|
|
||
| /* enterprise message aligned under heading, same left edge */ | ||
| .auth-hero-message { | ||
| max-width: 492px; | ||
| color: #fff; | ||
| font-family: 'Inter', sans-serif; | ||
| font-size: 22px; | ||
| font-weight: 400; | ||
| line-height: 36px; | ||
| } |
There was a problem hiding this comment.
The font-family 'Inter' is hardcoded in the styles but there's no guarantee this font is loaded in the application. If 'Inter' is not available, the browser will fall back to the generic sans-serif, which may not match the design. Verify that the 'Inter' font is properly loaded in the application, or use a CSS variable that references the application's font stack.
| {enterpriseWelcomeHtml && ( | ||
| <div | ||
| className="auth-hero-message mt-4" | ||
| dangerouslySetInnerHTML={{ __html: enterpriseWelcomeHtml }} | ||
| /> |
There was a problem hiding this comment.
Using dangerouslySetInnerHTML without sanitization is a security risk. The enterpriseWelcomeHtml contains HTML from enterpriseBrandedWelcomeString or platformWelcomeString which could potentially include malicious scripts. Consider sanitizing the HTML content using a library like DOMPurify before rendering it.
| {enterpriseWelcomeHtml && ( | ||
| <div | ||
| className="auth-hero-message mt-4" | ||
| dangerouslySetInnerHTML={{ __html: enterpriseWelcomeHtml }} | ||
| /> |
There was a problem hiding this comment.
Using dangerouslySetInnerHTML without sanitization is a security risk. The enterpriseWelcomeHtml contains HTML from enterpriseBrandedWelcomeString or platformWelcomeString which could potentially include malicious scripts. Consider sanitizing the HTML content using a library like DOMPurify before rendering it.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import messages from './messages'; | ||
|
|
||
| const SmallLayout = ({ fullName }) => { | ||
| const SmallLayout = () => { |
There was a problem hiding this comment.
The BaseContainer component passes a fullName prop to the layout components (SmallLayout, MediumLayout, LargeLayout), but these components have been refactored to no longer accept this prop. This will generate React PropTypes warnings in development and the fullName is never used. Either update BaseContainer to not pass fullName to these components, or add PropTypes validation to handle it gracefully.
| import messages from './messages'; | ||
|
|
||
| const MediumLayout = ({ fullName }) => { | ||
| const MediumLayout = () => { |
There was a problem hiding this comment.
The BaseContainer component passes a fullName prop to the layout components, but this component has been refactored to no longer accept this prop. This will generate React PropTypes warnings in development and the fullName is never used. Either update BaseContainer to not pass fullName to these components, or add PropTypes validation to handle it gracefully.
| import messages from './messages'; | ||
|
|
||
| const LargeLayout = ({ fullName }) => { | ||
| const LargeLayout = () => { |
There was a problem hiding this comment.
The BaseContainer component passes a fullName prop to the layout components, but this component has been refactored to no longer accept this prop. This will generate React PropTypes warnings in development and the fullName is never used. Either update BaseContainer to not pass fullName to these components, or add PropTypes validation to handle it gracefully.
| window.location = { hostname: getConfig().SITE_NAME, href: getConfig().BASE_URL }; | ||
|
|
||
| const props = { selectedPage: LOGIN_PAGE }; | ||
| render(reduxWrapper(<Logistration />)); |
There was a problem hiding this comment.
The component is being rendered twice, which may cause duplicate queries and unnecessary re-renders. The first render at line 283 doesn't use the return value, and the second render at line 284 captures the container. Remove the first render call at line 283.
| render(reduxWrapper(<Logistration />)); |
| })); | ||
| }; | ||
| const trackForgotPasswordLinkClick = () => { | ||
| sendTrackEvent('edx.bi.password-reset_form.toggled', { category: 'user-engagement' }); |
There was a problem hiding this comment.
Analytics tracking calls now omit the app_name parameter. The code changed from using the wrapper tracking functions (which automatically include APP_NAME) to calling sendTrackEvent directly without the app_name parameter. This creates inconsistency with other parts of the codebase that use the wrapper functions in src/data/segment/utils.js. For consistency and to ensure proper tracking, either use the existing wrapper functions or explicitly include app_name: APP_NAME in the tracking calls.
| sendTrackEvent('edx.bi.password-reset_form.toggled', { category: 'user-engagement' }); | |
| sendTrackEvent('edx.bi.password-reset_form.toggled', { | |
| category: 'user-engagement', | |
| app_name: getConfig().APP_NAME, | |
| }); |
| }, | ||
| 'with.edx': { | ||
| id: 'with.edx', | ||
| defaultMessage: ' with edX', |
There was a problem hiding this comment.
The message 'with.edx' is missing a description field. All other messages in this file have descriptions that explain their purpose. Add a description like "description: 'Second line of header text with edX branding for logistration MFE pages'" for consistency with the codebase conventions.
| defaultMessage: ' with edX', | |
| defaultMessage: ' with edX', | |
| description: 'Second line of header text with edX branding for logistration MFE pages', |
| {enterpriseWelcomeHtml && ( | ||
| <div | ||
| className="auth-hero-message mt-4" | ||
| dangerouslySetInnerHTML={{ __html: enterpriseWelcomeHtml }} |
There was a problem hiding this comment.
The use of dangerouslySetInnerHTML with enterpriseWelcomeHtml from backend data creates a Cross-Site Scripting (XSS) vulnerability. If the backend data is compromised or contains malicious content, it could execute arbitrary JavaScript in users' browsers. Consider using a sanitization library like DOMPurify to sanitize the HTML before rendering, or better yet, use a safer rendering approach that doesn't require raw HTML insertion.
|
|
||
| useEffect(() => { | ||
| trackLoginPageViewed(); | ||
| sendPageEvent('login_and_registration', 'login'); |
There was a problem hiding this comment.
Analytics tracking calls now omit the app_name parameter. The code changed from using wrapper functions like trackLoginPageViewed() and trackForgotPasswordLinkClick() (which automatically include APP_NAME) to calling sendPageEvent and sendTrackEvent directly without the app_name parameter. This creates inconsistency with other parts of the codebase that use the wrapper functions in src/data/segment/utils.js. For consistency and to ensure proper tracking, either use the existing wrapper functions or explicitly include app_name: APP_NAME in the tracking calls.
| <svg className="m1-n1 w-100 h-100 large-screen-svg-light" preserveAspectRatio="xMaxYMin meet"> | ||
|
|
||
| {/* keep existing right decorative triangle */} | ||
| <div className="col-md-3 bg-white p-0"> |
There was a problem hiding this comment.
The column width has changed from col-md-2 to col-md-3, which alters the layout proportions. The left column is still col-md-10, so the total is now 13 columns instead of 12, which exceeds Bootstrap's 12-column grid system. This will cause layout issues. Either change the left column to col-md-9 or change the right column back to col-md-2 to maintain a valid 12-column grid.
| <div className="col-md-3 bg-white p-0"> | |
| <div className="col-md-2 bg-white p-0"> |
| </div> | ||
| </h2> | ||
| <div className="auth-hero-heading-line text-accent-a"> | ||
| {formatMessage(messages['with.edx'])} |
There was a problem hiding this comment.
Inconsistent messaging across different layout sizes. SmallLayout and MediumLayout use messages['with.site.name'] with dynamic siteName, while LargeLayout uses messages['with.edx'] with hard-coded " with edX" text. This creates an inconsistent user experience where the message changes based on screen size. Use the same message pattern across all layouts for consistency, preferably messages['with.site.name'] to support different site configurations.
| {formatMessage(messages['with.edx'])} | |
| {formatMessage(messages['with.site.name'], { siteName })} |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 14 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| useEffect(() => { | ||
| const authService = getAuthService(); | ||
| if (authService) { | ||
| authService.getCsrfTokenService().getCsrfToken(getConfig().LMS_BASE_URL); | ||
| authService.getCsrfTokenService() | ||
| .getCsrfToken(getConfig().LMS_BASE_URL); |
There was a problem hiding this comment.
This useEffect has no dependency array, so it will request a CSRF token on every render. That can create unnecessary network traffic and flaky behavior; add an appropriate dependency array (likely []) so the token is fetched once on mount (or when LMS_BASE_URL changes).
| const handleInstitutionLogin = (e) => { | ||
| sendTrackEvent('edx.bi.institution_login_form.toggled', { category: 'user-engagement', app_name: APP_NAME }); | ||
| sendTrackEvent('edx.bi.institution_login_form.toggled', { category: 'user-engagement' }); | ||
| if (typeof e === 'string') { | ||
| sendPageEvent('login_and_registration', e === '/login' ? 'login' : 'register', { app_name: APP_NAME }); | ||
| sendPageEvent('login_and_registration', e === '/login' ? 'login' : 'register'); | ||
| } else { |
There was a problem hiding this comment.
These analytics calls were changed to omit the { app_name: APP_NAME } payload property. Elsewhere in the repo (e.g., src/data/segment/utils.js) tracking helpers always include app_name, so this makes event payloads inconsistent. Consider using the shared tracking helpers or continue passing app_name here for consistency.
| <RedirectLogistration | ||
| success={loginResult.success} | ||
| redirectUrl={loginResult.redirectUrl} | ||
| finishAuthUrl={finishAuthUrl} | ||
| currentProvider={currentProvider} | ||
| /> |
There was a problem hiding this comment.
ThirdPartyAuthAlert sets the ssoPipelineRedirectionDone cookie when currentProvider is present. Previously the login flow cleared that cookie on successful login; that cleanup (and the login-success tracking) is now gone, so the cookie may linger across sessions. Consider re-adding a useEffect on loginResult.success to track login success and clear ssoPipelineRedirectionDone after a successful login.
| props.dismissPasswordResetBanner(); | ||
| dispatch(dismissPasswordResetBanner()); | ||
| } | ||
|
|
There was a problem hiding this comment.
The sign-in submit handler no longer emits the Cohesion element-click event (compare with RegistrationPage, which still dispatches setCohesionEventStates). If Cohesion is still required for the sign-in button, please restore the Cohesion tracking dispatch in this handler.
| // Cohesion element-click tracking for the sign-in button | |
| sendTrackEvent('cohesion.element-click', { | |
| page: 'login', | |
| elementId: 'sign-in-submit', | |
| elementLabel: 'sign-in', | |
| }); |
| <div className="auth-hero-heading"> | ||
| <div className="auth-hero-heading-line text-white"> | ||
| {formatMessage(messages['start.learning'])} | ||
| </div> | ||
| <div className="auth-hero-heading-line text-accent-a"> | ||
| {formatMessage(messages['with.site.name'], { siteName })} | ||
| </div> | ||
| </h2> | ||
| </div> |
There was a problem hiding this comment.
The hero heading is now built with <div> elements instead of semantic heading tags (<h1>, <h2>, etc.). This reduces document structure information for screen readers; consider using appropriate heading elements (and styling them) to preserve accessibility semantics.
| @media (max-width: 1199.98px) { | ||
| .auth-hero-heading-line { | ||
| font-size: 2.5rem; | ||
| } | ||
|
|
||
| .auth-hero-content { | ||
| margin-top: 4rem; | ||
| } |
There was a problem hiding this comment.
This media query uses a hard-coded pixel breakpoint (1199.98px), while the rest of this stylesheet uses Paragon custom media breakpoints (e.g. @media (--pgn-size-breakpoint-min-width-xl)). For consistency and easier future updates, please use the same breakpoint tokens here.
| }, | ||
| 'with.edx': { | ||
| id: 'with.edx', | ||
| defaultMessage: ' with edX', |
There was a problem hiding this comment.
defaultMessage for with.edx includes a leading space (' with edX'). Since this string is rendered on its own line in LargeLayout, the leading space will be visible and may affect alignment; consider removing the leading space.
| defaultMessage: ' with edX', | |
| defaultMessage: 'with edX', |
| <div className="auth-hero-heading"> | ||
| <div className="auth-hero-heading-line text-white"> | ||
| {formatMessage(messages['start.learning'])} | ||
| </div> | ||
| <div className="auth-hero-heading-line text-accent-a"> | ||
| {formatMessage(messages['with.site.name'], { siteName })} | ||
| </div> | ||
| </h2> | ||
| </div> |
There was a problem hiding this comment.
This hero heading uses <div> elements rather than semantic headings. For accessibility and SEO, consider using <h1>/<h2> (or role="heading" with aria-level) so screen readers can correctly navigate the page structure.
| .auth-hero-heading-line.text-accent-a { | ||
| color: #03C7E8 !important; // for "with edX" | ||
| } | ||
|
|
There was a problem hiding this comment.
These heading styles hard-code font + colors and use !important for the accent color. This makes them harder to theme and override consistently; prefer existing typography tokens/utilities and avoid !important by adjusting specificity or using the provided Paragon text utility classes.
| .auth-hero-heading-line.text-accent-a { | |
| color: #03C7E8 !important; // for "with edX" | |
| } |
| expect(sendTrackEvent).toHaveBeenCalledWith('edx.bi.password-reset_form.toggled', { category: 'user-engagement', app_name: APP_NAME }); | ||
| expect(sendTrackEvent).toHaveBeenCalledWith( | ||
| 'edx.bi.password-reset_form.toggled', | ||
| expect.any(Object), |
There was a problem hiding this comment.
This assertion was weakened to expect.any(Object), which could let regressions slip (e.g., missing category: 'user-engagement'). Since the current implementation sends a stable payload shape, consider asserting the expected properties instead of any(Object).
| expect.any(Object), | |
| expect.objectContaining({ | |
| category: 'user-engagement', | |
| }), |
Enterprise theming changes