feat(guide): give every screen a tour that is about that screen - #526
Merged
Conversation
The Guide button opened the *dashboard* tour on every route it did not
recognise, so pressing it on Accounting welcomed the user to a dashboard they
were not looking at. Route matching was `url.includes('/loans')`, which claimed
any path with those characters anywhere in it.
Now each URL resolves through anchored patterns, most specific first, and a
screen with no hand-written copy gets a tour composed from its own route title
plus one step per control that is actually on it. A hand-written tour that
points at nothing gains those steps too, which took the section pages from one
card to four. No screen borrows another's copy, and no step describes something
that is not there.
Four selectors pointed at elements that cannot exist. Each rendered its copy and
highlighted nothing, which is invisible from the outside:
button[headerActions] the attribute sits on an ion-button, whose native
button is in a shadow root that never carries it
mat-select[name=...] Material leftovers; this application has no Angular
Material at all
.tab-group applied by no template anywhere — a dead style rule in
five record views, from the Material port. All sixteen
screens with tabs render an ion-segment
[headerActions] alone groups, centres and share accounts ask app-data-table
for a create button instead of projecting one
Steps are scoped: a content step is searched inside `main`, so a loose selector
can no longer reach the shell the way a bare `ul` once resolved to the sidebar's
nav list. Resolution retries while the view settles, rather than silently
finding nothing on a screen that is still fetching.
Walking all twenty guided views at 412x839 and at 1440x900 found three more:
- the business-date and Guide steps pointed at controls that move into the
header's overflow menu on a narrow viewport, so both highlighted nothing;
- the share-account form's last select sat 30px under the mobile sheet;
- on the dashboard at 1440x900 the card painted over 77.6% of the System Status
card — that step's entire subject, with 48px showing.
Both widths now get room to scroll a target clear of the card. Measured after:
0% covered, and no step at either width fails to find its target.
Escape listens on `document` rather than on the host. The tour is non-modal by
design, so a user who clicks the thing a step describes moved focus into the
page, and Escape then did nothing at all.
A tour also no longer survives navigation. Opening it on the dashboard and
clicking Clients used to leave the dashboard card over the client list, with
Next explaining Fineract environment health and nothing highlighted.
The highlight colour is a themed token instead of a hex in a component. One
value cannot serve both: against the surfaces a highlight lands on, #b45309 is
4.23-5.02:1 in light and only 3.32-3.87:1 in dark, while #f59e0b is 7.76-9.05:1
in dark and 1.81-2.15:1 in light. Measured live in dark mode afterwards: 7.76:1
and 8.72:1 for the outline, 7.69:1 for the card text.
The highlight no longer sets position or z-index, which was enough to move a
grid child while the tour was open, and "Step X of Y" and the button's own label
are translated rather than hardcoded English.
DOCS/GUIDED_TOUR.md covers how a tour is chosen, how one is composed, and the
selector traps above. DOCS/MOBILE.md said the narrow header hides the business
date and guide; it moves them into a popover, which is what the new grouped
selectors depend on.
`npm run ga:check` treats `.innerHTML` as a raw HTML sink wherever it appears under `src/`, and rejects it — a test is not exempt from a rule about sinks, and the gate is right not to carve out an exception it would then have to police. The fixtures only ever needed an element with a class on it, so an `el()` helper and `append` do the same job. The comment explaining this originally quoted the sink it was avoiding, which tripped the same pattern; it describes it instead. 8/9 gates pass; 0 blocking failure(s), 0 undetermined. The one remaining line is the advisory adapter-boundary backlog at 323, which `git diff apache/main...HEAD -- eslint-suppressions.json` shows this branch does not touch.
Contributor
E2E — mocked backend🎭 E2E Tests✅ All green — 347 passed · 0 failed · 1 skipped, across 28 spec files in 6m 52s. By spec file
All 348 tests — click to expand
Slowest 10 — what the shard counts should be tuned against
📼 Download the HTML report, videos and traces — see the Generated by run 34043950052 from |
Contributor
E2E — real Fineract🎭 E2E Tests✅ All green — 77 passed · 0 failed · 0 skipped, across 23 spec files in 6m 54s. By spec file
All 77 tests — click to expand
Slowest 10 — what the shard counts should be tuned against
📼 Download the HTML report, videos and traces — see the Generated by run 34043950052 from |
`feature-coverage.spec.ts` looks for the Guide button by the accessible name `/Help Tour|Guide/`, and it stopped finding it. The cause was not the renamed string: I had put the long form on an `aria-label`, which *replaces* visible text as the accessible name, so the button read "Guide" and answered to "Open the guided tour for this screen". That is WCAG 2.5.3 Label in Name. Someone driving the page by voice says "click Guide" and nothing happens, and the spec is exactly the check that catches it. The long wording is a description now — `appTooltip` wires `aria-describedby` — and the visible text is the name again, so the existing spec needs no change. feature-coverage.spec.ts -g "help tour button" 1 passed guidance-tour.spec.ts 5 passed accessibility.spec.ts 3 passed
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
1 participant
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.
The headline defect
The Guide button opened the dashboard tour on every route it did not recognise. Route matching was
url.includes('/loans'), which also claimed any path with those characters anywhere in it./accountingonmain— the copy reads "the key areas of the dashboard" over a screen that is not one:The same screen on this branch:
Both frames are the same screen at the same moment —
mainat55abe539was served alongside the branch.Every screen gets its own tour
URLs resolve through anchored patterns, most specific first. A screen with no hand-written copy gets a tour composed from its own route
titleplus one step per control that is actually on it, detected at open time. A hand-written tour that points at nothing gains those steps too, which took the section pages from one card to four.A step appears only because the control is there, so nothing is filler and nothing can go stale when a screen changes.
Four selectors pointed at elements that cannot exist
Each rendered its copy and highlighted nothing, which is invisible from the outside:
button[headerActions]ion-button, whose native button is in a shadow root that never carries itmat-select[name="productId"],[name="savingsAccountId"].tab-groupion-segment[headerActions]aloneapp-data-tablefor a create button instead of projecting oneSteps are also scoped now: a content step is searched inside
main, so a loose selector can no longer reach the shell the way a bareulonce resolved to the sidebar's nav list. Resolution retries while the view settles rather than silently finding nothing on a screen that is still fetching.Walking all twenty guided views found three more
At 412×839 and 1440×900, stepping every step and checking whether the declared target actually took the highlight:
Both widths now get room to scroll a target clear of the card. Re-measured: 0% covered, and no step at either width fails to find its target.
The narrow layout is a full-width bottom sheet at the shared 768px shell breakpoint:
Two defects found in review, in code added by this PR
Worth calling out rather than burying, since both were mine and both were caught by checking rather than by reading:
documentnow, guarded so it does not answer an Escape meant for something else.NavigationEnd.Both have a regression test that fails without the fix.
Accessibility, i18n and theming
The card is a
dialognamed and described by its own copy, the step counter is a polite live region because Next replaces the text in place, Escape closes, and focus returns to the opener."Step X of Y" and the button's own label were hardcoded English and are now translated.
The highlight colour is a themed token instead of a hex in a component. One value cannot serve both themes — measured against the surfaces a highlight lands on:
#b45309#f59e0bSo each theme takes the value that clears WCAG 1.4.11's 3:1 comfortably. Measured live in dark mode afterwards: 8.72:1 for the outline against the surface behind it, 7.69:1 for the card text.
The highlight also no longer sets
positionorz-index, which was enough to move a grid child while the tour was open.Verification
npm run test:unitnpm run buildmocked/mobilecheck:translations,check:responsive,check:a11y-names,check:icons,check-license.sh,check:branding-path,typecheck:e2eplaywright.config.tsgains aDUAL_VIEWPORT_SPECSlist rather than adding the spec toMOBILE_SPECS: that array doubles as themockedproject'stestIgnore, so joining it would have removed the desktop half of the spec from the run entirely.DOCS/GUIDED_TOUR.mddocuments how a tour is chosen, how one is composed, and the selector traps above.DOCS/MOBILE.mdsaid the narrow header hides the business date and guide — it moves them into a popover, which is what the new grouped selectors depend on, so that paragraph is corrected.Known and not addressed here
/tellersshows "This screen" rather than "Tellers": itspath: ''child declares notitle. 197 of 305 route entries have one; 285 routes in 21 feature files still inherit their tab title from the section above them #355 covers that.role="dialog"without a focus trap. Correct for a non-modal coach mark, but Tab still walks behind the card./accounting,/organization,/systemvisible in the first screenshot, and/security/audit-trailssilently redirecting to/dashboard. Both predate this branch and are filed separately.