Conversation
dschom
force-pushed
the
FXA-14234
branch
3 times, most recently
from
August 1, 2026 01:41
035fa45 to
e9640ad
Compare
Because: - After scanning the pairing QR code on their computer, users on mobile need a screen that tells them the sign-in still has to be approved on the computer, and that shows the requesting device's details so they can confirm the request is really theirs before pairing completes. This commit: - Adds `packages/fxa-settings/src/pages/Pair2/Supplicant/ApproveSignIn/index.tsx`, a presentational card rendering the Firefox brand logo, a desktop/mobile sync illustration, the heading and instruction copy, the device info block, and a Cancel action. It supplies its own card surface rather than using `AppLayout`, per the Pair2 mobile designs. `onCancel` is a required prop so that routing the card cannot leave its only action inert; the flow logic lands with the route. - Adds `device-info-lines.ts`, which selects the Fluent message for each line of the device info block as a pure function so the device-name/OS fallback and the five geo-resolution permutations are unit tested without rendering. The location and IP lines reuse the shipped `device-info-block-*` messages, so only the browser-on-device string is new. - Adds `en.ftl` with the new strings, `index.stories.tsx` with four stories covering the device-name, OS-fallback, unknown-location, and long-device-name cases, and tests for the component and the message selection. The component suite asserts wiring and runs `testAllL10n` against the bundle to catch drift between fallback text and the Fluent messages; the permutations live in the pure-function suite. - Exports `firefox-brand-logo.svg` and `sync-devices.svg` from Figma. The sync illustration is cropped to its visible bounds so it centres correctly, and the browser-tab favicon it embedded at 1046x1081 for a ~3px render is downscaled, taking the asset from 946KB to 58KB. Closes #FXA-14234
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Because
This pull request
packages/fxa-settings/src/pages/Pair2/Supplicant/ApproveSignIn/index.tsx, a presentational card rendering the Firefox brand logo, a desktop/mobile sync illustration, the heading and instruction copy, the device info block, and a Cancel action. The card surface comes fromAppLayout, matching the sibling Pair2 card in feat(settings): add mobile "Connect this device" pairing card #20954 —.cardonly paints a backdrop frommobileLandscapeup, so on a phone this still reads as the full-bleed mobile design. Device details arrive as aremoteMetadataprop rather than the component reading the pairing channel itself.fill="black"tocurrentColor(the fox glyph is all gradients, so only the lettering needed it) and renders it inlined, so it stays legible on the dark card surface. The device-info box getsdark:variants to match. Same one-line SVG change feat(settings): add mobile "Connect this device" pairing card #20954 made to its copy of the asset.onCancela required prop so routing this card cannot leave its only available action inert. Per the ticket, no button behaviour is wired up here — the handler lands with the route.components/DeviceInfoBlockrather than reimplementing it. The Figma frame shows "Firefox on Laurels-MacBook-Pro" — the device name folded into the browser line, no heading — which is a third presentation the component did not have, soshowDeviceName-style booleans do not cover it. AddsdeviceNameDisplay?: 'heading' | 'inline' | 'hidden'(defaultheading) plusclassName?: string(default'mt-8 mb-4'), both defaulted to today's behaviour so none of the four livePair/*callers change. All three modes fall back to the OS line when the payload carries no device name, which is whatpairing-authority-integrationproduces on the authority pages.DeviceInfoBlock/en.ftlasdevice-info-browser-device, alongside thedevice-info-browser-osit parallels. Net zero new strings versus the previous approach, but the id is now owned by the component that renders it instead of being borrowed across a package boundary.components/imagescollection —graphic_firefox_wordmark.svgandgraphic_sync_devices.svg— behindFirefoxWordmarkImageandSyncDevicesImagewrappers, following the directory's existingPreparedImagepattern. FXA-14235 through FXA-14247 render the same lockup and illustration, so their aria-labels are localized once here (firefox-wordmark-image-aria-label,sync-devices-image-aria-label) rather than redefined per page. The card imports the two wrappers and needs no image markup, asset, oruseFtlMsgResolverof its own. Both are added to theComponents/Imagesstorybook page.en.ftl,index.stories.tsxwith four stories (device name, OS fallback, unknown location, long device name), and a component test suite that asserts wiring and checks every message this page owns against the Fluent bundle. The device-name and geo permutations are covered inDeviceInfoBlock's own suite, which gains tests for the two new modes, the no-device-name fallback, and theclassNameoverride.Issue that this pull request solves
Closes: FXA-14234
Checklist
Put an
xin the boxes that applyHow to review (Optional)
components/DeviceInfoBlock/index.tsx— it is the only touched file with existing callers. The new card'sindex.tsxshould read as pure markup.DeviceInfoBlock/index.tsx(smallest, shared) → its tests → the card'sindex.tsx→en.ftl→ stories/tests.Pair/AuthAllow,Pair/SuppAllow,Pair/AuthWaitForSupp,Pair/SuppWaitForAuth. Both new props default to current behaviour and no call site changed, but that is the claim worth checking. The new card itself is still unrouted and cannot regress anything.currentColorwordmark and the device-info box, via the Storybook theme toolbar.Screenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
viewBoxis cropped to the visible content. It also embedded the browser-tab favicon at 1046x1081 for a ~3px render; downscaling that payload took the asset from 946KB to 58KB./fxa-review-quickreturned APPROVE with 0 blocking issues. Folded in: thetestAllL10nbundle check, requiredonCancel, and trimming component tests that duplicated coverage held elsewhere.AppLayoutresolved two things previously deferred here: the<main>landmark now comes from the layout, and the card inherits.card's dark treatment (withdark:variants added for the wordmark and device-info box). Still deferred: the document title, which belongs to whoever wires the route and should be passed asAppLayout'stitleprop.AppLayoutrenders a Mozilla logo in the page header while the design puts a Firefox lockup inside the card, so both appear. The desktop sibling frames have no in-card logo, which suggests the mobile Pair2 screens are meant to drop the standard header — a layout call for the flow-wiring ticket.device-info-lines.tsthat duplicatedDeviceInfoBlock's location branching and borrowed FTL ids it owned, with aTODOto consolidate later. That is gone — the logic lives in the shared component and the duplicate module and its 137-line suite are deleted, so this PR is a net reduction in the pairing code it touches.showDeviceName?: booleantoDeviceInfoBlockfor the same box; this one adds thedeviceNameDisplayenum, whose'hidden'covers the identical case. Two interacting booleans on a shared component read badly, hence the enum — but whichever lands second has to reconcile. For feat(settings): add mobile "Connect this device" pairing card #20954 that is a one-line change fromshowDeviceName={false}todeviceNameDisplay="hidden".DeviceInfoBlock/index.stories.tsxspreadPartial<RemoteMetadata>as JSX props instead of merging intoremoteMetadata. TypeScript skips excess-property checks on spreads, so it compiled while silently rendering nothing — theWithDeviceNamestory never showed a device name. The new mode stories use that helper, so it was corrected rather than built on.testAllL10ndoes not work on pages with a labelledPreparedImage, so this page filters rather than calls it. Jest stubs SVG imports with a component whose text content is the file name, so an attribute-only message (.aria-label = Firefox) can never match its rendered text and the check always throws. Every page that hits this resolves it by commentingtestAllL10nout wholesale —Pair/Success,ConnectAnotherDevice,SigninTokenCode,Signup/ConfirmSignupCode. Here the shared-image ids are skipped and asserted against the bundle directly instead, so the rest of the page keeps real coverage. Worth a follow-up ticket against the harness; not fixed here.FirefoxWordmarkImage/SyncDevicesImage, which also upgrades its illustration from the PNG to the SVG. The device-info box is likewise shared now viaDeviceInfoBlock.