Skip to content

refactor(auth): make more generic AuthAside, and AuthPageFrame DEV-1850 - #7612

Open
magicznyleszek wants to merge 3 commits into
mainfrom
leszek/dev-1850-profile-details-ui-blocker-cleanup-2
Open

magicznyleszek wants to merge 3 commits into
mainfrom
leszek/dev-1850-profile-details-ui-blocker-cleanup-2

Conversation

@magicznyleszek

@magicznyleszek magicznyleszek commented Sep 18, 2026 •

Copy link
Copy Markdown
Member

🗒️ Checklist

  1. run linter locally
  2. update developer docs (API, README, inline, etc.), if any
  3. for user-facing doc changes create a Zulip thread at #Support Docs Updates, if any
  4. draft PR with a title <type>(<scope>)<!>: <title> DEV-1234
  5. assign yourself, tag PR: at least Front end and/or Back end or workflow
  6. fill in the template below and delete template comments
  7. review thyself: read the diff and repro the preview as written
  8. open PR & confirm that CI passes & request reviewers, if needed
  9. act on any greptile review below a 5/5 score or leave comment explaining why you won't
  10. delete this checklist section from the final squash commit before merging

💭 Notes

Groundwork for a route blocker - it will add a "complete your profile details" screen that has to look like the other authentication screens but is reached after login, outside the /auth routes. Two pieces it needs were split out:

  • AuthPageFrame - the background, logo, language picker and legal links, lifted out of AuthContainer. AuthContainer is now just the /auth layout route: the frame around an <Outlet/>.
  • AuthAside - RegisterRoute/RegisterAside made generic and moved next to AuthCard, plus shouldRenderAuthAside() so a screen can decide whether to open the column at all.
  • useEnvironmentQuery - to avoid making multiple calls to environment endpoint

Pure refactor, no behavior change intended.

👀 Preview steps

  1. ℹ️ needs the authRedesignEnabled feature flag on
  2. go to /#/auth/register
  3. 🟢 notice the background, logo, language picker and Terms/Privacy links are unchanged from main
  4. ℹ️ in Django admin, clear both the login supporting image and the welcome_message sitewide message
  5. 🟢 notice the card is one column wide, with no divider
  6. ℹ️ set either one back
  7. 🟢 notice the supporting column returns, and the card widens as before
  8. 🟢 notice tab order through logo → language picker → form → footer links is unchanged

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The refactor preserves existing behavior and consistently updates the affected components and references.

Pull request overview

Refactors authentication layouts and supporting content into reusable components while preserving existing registration behavior.

Changes:

  • Extracts the shared authentication page frame.
  • Generalizes the registration aside as AuthAside.
  • Updates registration and Storybook references.
File summaries
File Description
jsapp/js/auth/RegisterRoute/RegisterRoute.tsx Uses the generic authentication aside.
jsapp/js/auth/RegisterRoute/RegisterAside.tsx Removes the registration-specific component.
jsapp/js/auth/AuthContainer/AuthPageFrame.tsx Adds the reusable authentication page frame.
jsapp/js/auth/AuthContainer/AuthPageFrame.module.scss Styles the extracted page frame.
jsapp/js/auth/AuthContainer/AuthContainer.tsx Wraps routed screens with the shared frame.
jsapp/js/auth/AuthContainer/AuthAside.tsx Adds the generic supporting-content component.
jsapp/js/auth/AuthContainer/AuthAside.stories.tsx Updates stories for the generic aside.
jsapp/js/auth/AuthContainer/AuthAside.module.scss Styles the generic aside content.
Review details
  • Files reviewed: 6/8 changed files
  • Comments generated: 0
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@magicznyleszek
magicznyleszek marked this pull request as ready for review September 18, 2026 16:36
@greptile-apps

greptile-apps Bot commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5 Tier: plus

[Medium risk] Refactors authentication UI components for reuse.

The PR’s behavior appears safe, but the explicit translation requirement for the logo’s accessible label should be satisfied before merging.

Summary

The PR extracts a reusable authentication page frame and supporting aside, and shares /environment query settings across the frame and language selector.

  • Registration retains its conditional supporting column.
  • The extracted frame retains the existing layout and legal links, but its logo alt text does not meet the repository’s translation requirement.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
  AuthContainer --> AuthPageFrame
  AuthPageFrame --> Outlet
  AuthPageFrame --> useAuthConfiguration
  AuthPageFrame --> StandaloneUILanguageSelector
  useAuthConfiguration --> useEnvironmentQuery
  StandaloneUILanguageSelector --> useEnvironmentQuery
  Outlet --> RegisterRoute
  RegisterRoute --> AuthCard
  AuthCard --> AuthAside
Loading

Reviews (7) · Last reviewed commit: "cr fix"

@magicznyleszek
magicznyleszek force-pushed the leszek/dev-1850-profile-details-ui-blocker-cleanup-2 branch from 1304a4b to 7ceae4a Compare September 22, 2026 07:37
@jamesrkiger

Copy link
Copy Markdown
Contributor

I don't think it's directly caused by this PR, but I notice when loading the registration page I'm hitting the /environment endpoint four times. I think we need to configure the staletime on the hook for that API call. Can you look into that?

@jamesrkiger jamesrkiger left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Left a comment about us hitting the environment endpoint too much. Let's resolve that in this PR stack, but I'll leave it up to you which PR to do it in. Otherwise LGTM

@magicznyleszek

Copy link
Copy Markdown
Member Author

I don't think it's directly caused by this PR, but I notice when loading the registration page I'm hitting the /environment endpoint four times. I think we need to configure the staletime on the hook for that API call. Can you look into that?

Added some code to fix that :)

Comment thread jsapp/js/api/useEnvironmentQuery.ts Outdated
Base automatically changed from leszek/dev-1850-profile-details-ui-blocker-cleanup to main September 28, 2026 18:08
@magicznyleszek
magicznyleszek force-pushed the leszek/dev-1850-profile-details-ui-blocker-cleanup-2 branch from 69305d0 to 32c9bc4 Compare September 28, 2026 18:08
<header className={styles.header}>
{authConfiguration?.show_kobotoolbox_logo && (
<a className={styles.logoLink} href='/'>
<img className={styles.logo} src={logoUrl} alt='KoboToolbox' />

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 Untranslated logo label

The extracted page frame uses the hardcoded KoboToolbox alt text for its logo. Screen readers announce this label, so it is user-facing text that the repository requires to be wrapped in t(). Please satisfy that requirement before merging.

Suggested change
<img className={styles.logo} src={logoUrl} alt='KoboToolbox' />
<img className={styles.logo} src={logoUrl} alt={t('KoboToolbox')} />

Rule Used: Only flag a hardcoded string as needing translation when it is user-facing: rendered in the React UI, or returned in an API/HTTP response body that end users or the frontend display (DRF ValidationError messages, default_detail, serializer er... (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants