IS-11218 Render BankID QR accessibility messages as collapsible sections#253
IS-11218 Render BankID QR accessibility messages as collapsible sections#253aleixsuau wants to merge 2 commits into
Conversation
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds frontend support for BankID QR-code accessibility messages delivered via metadata.viewData.messages, rendering them in the built-in BankID UI as two collapsible <details> sections to match the classic Velocity layout.
Changes:
- Introduces
HaapiStepperBankIdQrAccessibilityMessagesto render “instruction.” and “screen-reader.” message groups as collapsible sections. - Adds message-key normalization via
getQrViewDataMessagesto resolve keys by their.view.qr.<suffix>tail. - Extends HAAPI step typings with
metadata.viewData.messages, adds styling and documents the new CSS hook.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/haapi-react-sdk/haapi-stepper/util/qr-view-data-messages.ts | Adds normalization helper for .view.qr.-scoped message keys. |
| src/haapi-react-sdk/haapi-stepper/README.md | Documents the new .haapi-stepper-bankid-qr-accessibility CSS hook. |
| src/haapi-react-sdk/haapi-stepper/feature/viewnames/index.ts | Exports the new BankID QR accessibility messages component. |
| src/haapi-react-sdk/haapi-stepper/feature/viewnames/HaapiStepperBankIdQrAccessibilityMessages.tsx | Implements the collapsible accessibility sections for BankID QR messages. |
| src/haapi-react-sdk/haapi-stepper/feature/viewnames/HaapiStepperBankIdQrAccessibilityMessages.spec.tsx | Adds test coverage for rendering and toggle behavior. |
| src/haapi-react-sdk/haapi-stepper/feature/viewnames/BankIdViewNameBuiltInUI.tsx | Wires the new component into the built-in BankID view. |
| src/haapi-react-sdk/haapi-stepper/data-access/types/haapi-step.types.ts | Adds HaapiViewData and types metadata.viewData (incl. messages). |
| src/haapi-react-app/src/shared/util/css/styles.css | Styles the new container and <details> spacing/behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * View-specific data attached to the response. The shape depends on the view that produced it; | ||
| */ |
There was a problem hiding this comment.
It's a detail, but good to fix.
|
I tested locally and overall looks nice! One thing I noticed is that the progress bar is displayed bellow the new collapsible sections, which becomes quite off, name when the panels are expanded. I think we need to fix this. As mentioned in other comments, I also think that it would be better to contain the processing of this stuff in the BankID-specific view/code. For these reasons, I'm not approving yet. |
| /** | ||
| * View-specific data attached to the response. The shape depends on the view that produced it; | ||
| */ |
There was a problem hiding this comment.
It's a detail, but good to fix.
| * during step-data formatting. Present only on the QR-code link when the server supplied a | ||
| * complete section. | ||
| */ | ||
| qrCodeAccessibility?: HaapiStepperQrCodeAccessibility; |
There was a problem hiding this comment.
I see what you tried to do here, but conceptually, it's kind of weird that a link has "accessibility details".
I think this is too specific to BankID to be defined here. Also, because It means the formatting layer:
- Assumes some structure about the messages (see comments there) which I think are a bit of a stretch.
- Assumes the messages apply to a certain link, but they don't. They are something for the whole view.
I think we should not model such a strong relation now, because we don't have other use cases (YAGNI and generalizing too early). We are defining a bigger API surface that we need to support later on. If this is done in the BankID "built in UI", is much more contained to start with.
| * after `.view.qr.` (e.g. `instruction.heading`, `screen-reader.step4.2.1`). Keying by the suffix | ||
| * keeps this independent of the message-key prefix; keys without the marker are ignored. | ||
| */ | ||
| function stripQrCodeViewPrefix(messages?: Record<string, string>): Record<string, string> { |
There was a problem hiding this comment.
This is a bit of a stretch. The server returns messages with full keys to be precise and future proof for other added messages (e.g. partially matching). View data messages are specific to a certain representation (i.e. to a certain viewName).
We don't know anything about other possible use cases. Why would we assume this message structure could be reused/similar in other cases? I don't see the need for this assumption and generalization.
The logic to check for message presence and building typed sections is great (and extracting it to a dedicated file), but I suggest doing this in the BankID-specific namespace.
We can always generalize later, but don't compromise for now.
(you know I'm more conservative when it comes to this kind of thing 😃 but I think I presented good arguments)
Jira: https://curity.atlassian.net/browse/IS-11218
Frontend counterpart to the BankID server work (server PR #10231) that adds QR-code accessibility messages under
metadata.viewData.messages.What
Renders the BankID QR accessibility messages in
BankIdViewNameBuiltInUIas two collapsible<details>sections — "Help with scanning the QR code" (instruction.*) and "If you are using a screen reader" (screen-reader.*) — mirroring the classic Velocity layout.Test instructions
BankID