fix: w3ds messaging related UI Changes #348
Conversation
WalkthroughLogin screen UI standardization across three platforms addresses visual inconsistencies by reducing disclaimer prominence through smaller fonts and lighter text colors, center-aligning text blocks, adjusting typography, and repositioning the W3DS logo in Blabsy. Changes are presentation-only with minor formatting cleanups in utility files. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platforms/group-charter-manager/src/components/auth/login-screen.tsx (1)
171-179: Verify Group Charter–specific login screen tasks
- ✅ Single W3DS logo present and centered
- ✅ “60 seconds” validity text uses bold, block styling
- ✅ Disclaimer block is centered with
text-xsand gray styling⚠️ eID instruction<p>is onlytext-gray-600—addtext-smandtext-centerfor higher prominence
🧹 Nitpick comments (3)
platforms/group-charter-manager/src/components/auth/login-screen.tsx (3)
122-125: Reduce eID instruction font on small screens to avoid line breaks.Make the copy responsive so it shrinks on narrow viewports, per the issue requirements.
- <p className="text-gray-600"> + <p className="text-sm md:text-base text-gray-600"> Scan the QR code using your <a href={getAppStoreLink()}><b><u>eID App</u></b></a> to login </p>
157-158: Fix split closing tag.Minor JSX formatting nit to keep tags well-formed on one line.
- <span className="block font-light text-gray-600">Please refresh the page if it expires</span - > + <span className="block font-light text-gray-600">Please refresh the page if it expires</span>
167-167: US English spelling (optional): “centralized” vs “centralised”.If the product copy targets US English, prefer “centralized”.
- is stored in your own sovereign eVault, not on centralised + is stored in your own sovereign eVault, not on centralized
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
platforms/group-charter-manager/src/components/auth/login-screen.tsx(1 hunks)
🔇 Additional comments (1)
platforms/group-charter-manager/src/components/auth/login-screen.tsx (1)
162-169: Disclaimer styling aligns with Issue #347 (centered, smaller, lower-contrast).The update to text-center, text-xs, and text-gray-500 de-emphasizes the disclaimer appropriately while keeping it readable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
platforms/pictique/src/routes/(auth)/auth/+page.svelte (3)
84-84: Copy nit: use “log in” (verb) instead of “login” (noun).Also optional: prefer semantic
<strong>/CSS underline over<b>/<u>for consistency with Tailwind utilities.- Scan the QR code using your <b><u class="text-sm">eID App</u></b> to login + Scan the QR code using your <b><u class="text-sm">eID App</u></b> to log in
137-142: Lower-contrast disclaimer may miss WCAG AA at small sizes; consider slight bump.Keeps de-emphasis but improves readability on variable backgrounds.
- <p class="w-full rounded-md bg-white/60 p-4 text-center text-xs leading-4 text-black/40"> + <p class="w-full rounded-md bg-white/80 p-4 text-center text-xs leading-4 text-black/60">
144-146: Addrel="noopener noreferrer"to external link opened in a new tab.Prevents
window.openeraccess.- <a href="https://metastate.foundation" target="_blank"> + <a href="https://metastate.foundation" target="_blank" rel="noopener noreferrer">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
platforms/pictique/src/routes/(auth)/auth/+page.svelte(2 hunks)
🔇 Additional comments (1)
platforms/pictique/src/routes/(auth)/auth/+page.svelte (1)
90-95: Verify mobile tap target after font-size reduction.
text-baselowers label size; withpy-4the height should still be ~48px. Please confirm the button meets the 44×44px minimum on iOS/Android.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platforms/blabsy/src/components/login/login-main.tsx (1)
20-30: Close SSE and add cleanup to prevent leaks/duplicate sign-ins
EventSourcestays open indefinitely; close it after successful sign-in and on error.Apply within this block:
- eventSource.onmessage = async (e): Promise<void> => { - const data = JSON.parse(e.data as string) as { token: string }; - const { token } = data; - console.log(token); - await signInWithCustomToken(token); - }; + eventSource.onmessage = async (e): Promise<void> => { + try { + const data = JSON.parse(e.data as string) as { token: string }; + const { token } = data; + await signInWithCustomToken(token); + } finally { + eventSource.close(); + } + }; + eventSource.onerror = (): void => { + eventSource.close(); + };Outside this range, keep it robust across unmounts:
// add to imports import { useEffect, useState, useRef } from 'react'; // inside component const esRef = useRef<EventSource | null>(null); // in watchEventStream, after creating the EventSource esRef.current = eventSource; // in useEffect, add cleanup useEffect(() => { getOfferData().catch(console.error); return () => esRef.current?.close(); }, []);
🧹 Nitpick comments (7)
platforms/blabsy/src/components/login/login-main.tsx (7)
14-17: Prefer relative SSE URL to avoid brittle env depsAvoid hard reliance on
NEXT_PUBLIC_BASE_URLon the client; relative paths work across dev/prod.- const sseUrl = new URL( - `/api/auth/sessions/${id}`, - process.env.NEXT_PUBLIC_BASE_URL - ).toString(); + const sseUrl = `/api/auth/sessions/${id}`;
31-37: Same here: fetch offer via relative pathSimplifies config and prevents runtime errors if the env var is unset.
- const { data } = await axios.get<{ uri: string }>( - new URL( - '/api/auth/offer', - process.env.NEXT_PUBLIC_BASE_URL - ).toString() - ); + const { data } = await axios.get<{ uri: string }>('/api/auth/offer');
93-99: Disable the deep-link CTA untilqris readyPrevents navigating to
#and improves a11y.- <a - href={qr ? getDeepLinkUrl(qr) : '#'} - className='px-6 py-3 bg-blue-600 text-white rounded-lg hover:bg-blue-700 transition-colors text-center' - > + <a + href={qr ? getDeepLinkUrl(qr) : undefined} + aria-disabled={!qr} + tabIndex={qr ? 0 : -1} + onClick={(e) => { if (!qr) e.preventDefault(); }} + className={`px-6 py-3 bg-blue-600 text-white rounded-lg transition-colors text-center ${qr ? 'hover:bg-blue-700' : 'opacity-50 pointer-events-none'}`} + >
100-105: Fix split closing tagMinor JSX formatting glitch; keep the closing
</span>on one line.- <span className="block font-light text-gray-600">Please refresh the page if it expires</span - > + <span className="block font-light text-gray-600">Please refresh the page if it expires</span>Also applies to: 116-121
79-83: Arbitrarycontent:string quoting nitTo avoid parser/minifier quirks with smart quotes, prefer single-quoted Tailwind arbitrary values.
- <h1 className='text-center text-3xl before:content-["See_what’s_happening_in_the_world_right_now."] lg:text-6xl lg:before:content-["Happening_now"]'> + <h1 className='text-center text-3xl before:content-[\'See_what’s_happening_in_the_world_right_now.\'] lg:text-6xl lg:before:content-[\'Happening_now\']'>Optional: render the visible copy as real text instead of
::beforefor better i18n/theming.
125-132: Delete commented-out W3DS logo blockSince the second logo is intentionally removed, drop dead code instead of commenting it out.
- {/* <div className='absolute right-0 rotate-90 top-1/2'> - <Image - src='/assets/w3dslogo.svg' - alt='W3DS logo' - width={100} - height={20} - /> - </div> */}
135-141: Disclaimer still looks emphasized; tone it down per Issue #347Remove bold and reduce size/contrast to truly de-emphasize.
- <div className='grid gap-3 text-center font-bold text-white/70'> + <div className='grid gap-3 text-center font-normal text-white/60 text-xs md:text-sm'>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
platforms/blabsy/src/components/login/login-main.tsx(2 hunks)
🔇 Additional comments (4)
platforms/blabsy/src/components/login/login-main.tsx (4)
101-104: Confirm the “60 seconds” validity matches backend TTLMake sure the offer/session actually expires in 60s across platforms to avoid misleading copy.
Also applies to: 117-120
77-77: Alignment update LGTMCentering the right pane on large screens matches the issue’s alignment requirement.
84-86: Centered subheader LGTMThis aligns with the “center-align” requirement.
44-53: Guard browser-only APIs for SSR safetynavigator/window can be undefined during SSR — guard and return a safe default in non-browser environments. If this component is used in the Next.js App Router, add "use client" at the top of the file.
- const getAppStoreLink = () => { - const userAgent = navigator.userAgent || navigator.vendor || window.opera; + const getAppStoreLink = (): string => { + if (typeof window === 'undefined' || typeof navigator === 'undefined') { + return "https://play.google.com/store/apps/details?id=foundation.metastate.eid_wallet"; + } + const userAgent = navigator.userAgent || navigator.vendor || (window as any).opera;Verification note: root package.json showed no "next" (jq returned null) and a search found no "use client" in this file — confirm whether this package uses Next.js/App Router before adding the directive.
coodos
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
platforms/blabsy/src/components/login/login-main.tsx (1)
195-200: Consider minor text polish for clarity.The disclaimer text effectively communicates the W3DS architecture. For slightly improved readability, consider:
- You are entering Blabsy - a social network built on the - Web 3.0 Data Space (W3DS) architecture. This system is - designed around data-platform separation, where all your - personal content is stored in your own sovereign eVault, - not on centralized servers. + You are entering Blabsy—a social network built on the + Web 3.0 Data Space (W3DS) architecture. This system uses + data-platform separation, where your personal content is + stored in your own sovereign eVault, not on centralized + servers.This tightens the phrasing slightly (em dash instead of hyphen, "uses" instead of "is designed around", removes "all" for conciseness), but the current version is already clear and acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
infrastructure/eid-wallet/src/routes/(auth)/e-passport/+page.svelte(1 hunks)platforms/blabsy/src/components/login/login-main.tsx(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- infrastructure/eid-wallet/src/routes/(auth)/e-passport/+page.svelte
🧰 Additional context used
🧬 Code graph analysis (1)
platforms/blabsy/src/components/login/login-main.tsx (3)
platforms/dreamSync/client/src/lib/utils/mobile-detection.ts (1)
getAppStoreLink(29-45)platforms/blabsy/src/lib/utils/mobile-detection.ts (1)
getDeepLinkUrl(11-13)platforms/pictique/src/declaration.d.ts (1)
QRCode(3-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (5)
platforms/blabsy/src/components/login/login-main.tsx (5)
54-65: LGTM! Clean data fetching logic.The simplified
getOfferDatafunction correctly fetches the offer URI and initializes the SSE watcher without unnecessary logging.
86-90: LGTM! Simplified error handling.The useEffect now uses a cleaner
.catch()pattern for error handling without the noise of success logs.
94-117: LGTM! Proper centering aligns with PR objectives.The layout changes correctly center-align the headings and content wrapper on large screens, matching the requirement to make text blocks centered including "Happening now" and "Join Blabsy today".
129-189: LGTM! Login flows meet UI consistency objectives.The mobile and desktop login flows now properly:
- Use smaller, centered text for instructions
- Add prominent "60 seconds" validity notices as requested
- Include app store links for user guidance
The changes align well with the PR objectives to increase prominence of eID and time-related information.
192-214: Text contrast meets WCAG AA accessibility standards.Verification confirms the disclaimer text at
text-white/70achieves a contrast ratio of 5.99:1 against thebg-white/20background, exceeding WCAG AA requirements (4.5:1 for normal text, 3:1 for large text). The original concern about potential WCAG compliance failure was unfounded.However, verify visually in the rendered application to account for the underlying background image, which may affect the final perceived contrast. Since the theoretical calculation meets accessibility standards, the code changes are acceptable pending visual confirmation.
Description of change
Fixed metastate platforms requested changes
Issue Number
closes #347
Type of change
How the change has been tested
CI and Manual
Change checklist
Summary by CodeRabbit