Feat/docs feedback widget - #422
Merged
Merged
Conversation
…-page link DocItem/Footer --wrap swizzle appends a 'Was this page helpful?' widget on every doc page. Vote -> docs_page_feedback; free-text comment -> docs_page_feedback_comment (the actionable part). Uses the existing PostHog capture helper; one vote per session; client-only, no new deps. Also enables editUrl so pages get an edit/PR link (repo is private, so useful for contributors with access). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Keeps the per-page feedback widget; drops only the edit link. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
moritzhartmeier
left a comment
moritzhartmeier
left a comment
Member
There was a problem hiding this comment.
Reviewed the diff. Net change is 3 new files (+252/-0) — the second commit reverts the editUrl addition, so docusaurus.config.ts ends up untouched. All three CI checks pass.
Overall this is clean and well-scoped. No new dependencies, reuses the existing capturePostHogEvent helper, snake_case event properties consistent with docs_search_performed / docs_search_result_click, single-quote TSX style matching SkillsCallout, and English-only strings are fine given locales: ["en"]. I also verified the wiring actually takes effect: the existing ejected swizzle at src/theme/DocItem/Layout/index.js:53 renders <DocItemFooter />, so the --wrap swizzle is picked up rather than silently bypassed.
Comments inline. The three I'd want addressed before merge are the unbounded votes, the silent drop when PostHog is unavailable, and the dead vote buttons after sending — the rest are nits.
Two questions that aren't really code review:
- Who actually watches
docs_page_feedback_commentin PostHog? The UI copy promises the docs team reads it. Is there an alert, a dashboard, or a routine — or will these accumulate unread? This determines whether the widget is worth its footprint on every page. - Requiring a vote before the comment box appears is deliberate per the docstring. Is that the intended funnel, or should a reader be able to leave a comment without voting first? Right now the actionable signal is gated behind the less actionable one.
Twelve review rounds on the "Was this page helpful?" widget, driven in a real browser. The headline problem was not styling: COMMENTS WERE BEING THROWN AWAY UNDER A THANK-YOU. PostHog arrives through GTM behind the cookie banner, so on a first page view `window.posthog` is simply absent - and `capturePostHogEvent` was `ph?.capture?.()`, which swallows that. A reader wrote a comment, pressed Send, and read "it goes straight to the docs team" while nothing had left the page. The helper now returns whether the event was accepted (and catches a throwing capture, which used to escape a click handler and leave the UI inert), the widget branches on that, and an undelivered comment says so and keeps the reader's text. An explicit `has_opted_out_capturing()` counts as not accepted; the doc comment says plainly what `true` does and does not mean, since a stub-then-blocked script cannot be detected from here. VOTES WERE LOST THE SAME WAY, and worse, because a vote is fire-and-forget: the first-page-view case is exactly the reader who engaged, so the helpful-rate KPI lost them systematically. A dropped vote is now held and retried - when the comment is sent, on a bounded poll, and on unmount, which is the exit that matters because on a docs site people leave by clicking an in-site link and `pagehide` never fires for that. EVENTS COULD NOT BE JOINED TO PAGEVIEWS. `metadata.permalink` has no trailing slash while the site sets `trailingSlash: true`, so `url` was `/x` where `$pathname` is `/x/` and the join matched nothing. Also fixed: the page's identity now comes from doc metadata rather than `document.title`, which still holds the PREVIOUS page's value during a client-side navigation - both at render and at click time, so reading it later was no fix either. Join on `url`, not on `$pathname`: a vote flushed on the way out fires after the route change, and PostHog fills those in from `window.location`. The vote count now matches what the file always claimed. "One vote per browser session per page, so it can't be spammed" was in the header while six clicks sent six events; it is enforced through sessionStorage, and only a successful capture is recorded, so a reader who accepts cookies later is still heard. ACCESSIBILITY, all measured rather than reasoned about: a live region that is always mounted, because one inserted with its content announces nothing; focus moved to the confirmation, because submitting unmounts the textarea and focus fell to <body>; exactly one channel per message, since a newly mounted paragraph is read by the focus move and announcing as well said the same thing twice; the vote buttons `aria-disabled` rather than `disabled`, which dropped keyboard focus; a real focus ring instead of `outline: none`; and the 1,000 character cap disclosed and announced instead of silently truncating a paste. Contrast, from the built CSS: the selected vote was 3.26:1 (`-darker` on `-contrast-background`, where Infima's partner is `-contrast-foreground`) and is 12.98:1; the privacy note was 3.06:1 and is 5.72:1; the spent vote option is dimmed through its border rather than `opacity`, which had composited its label to 3.79:1. The card also used `--ifm-background-surface-color`, the one Infima default this site never overrides, so it was grey on a navy page. KNOWN LIMITS: When a truncating paste lands the box on exactly the cap, the "did not all fit" notice and the "that is the limit" notice are written to the same atomic region in one task, so only the second is announced. Consistent, but the reader is told about the limit and not about the truncation. The paste notice deliberately quotes no numbers. Character counts disagree three ways - code units, code points, grapheme clusters - and Blink backs off a surrogate pair rather than split it, so every exact figure tried was falsifiable. The visible counter states the position. `true` from the capture helper means PostHog accepted the event, not that it reached the server. `src/components/SkillsCallout/analytics.ts` is in this change: the shared helper's return type widens from `void` to `boolean`. Its three existing callers use it as a statement and are unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The contrast fix landed on --ifm-color-success/warning, which are Infima
stock and are never mapped to the brand anywhere in the repo. The site
already has a positive/negative pair with an agreed treatment - the tip and
warning admonitions - so these take that recipe verbatim from
doc-admonition.scss rather than inventing a second one:
.on --light-green-60 / --light-green-40 / --light-green-100
--dark-green-80 / --dark-green-100 / --dark-green-60
.bad --light-yellow-80 / --light-yellow-40 / (base label, as warning does)
--dark-brown-60 / --dark-brown-80 / --dark-brown-40
.failed follows the same warning recipe, by way of DesignLibraryNotice,
which is the closest thing the site has to this notice already.
The yellow ramp redefines itself for the dark theme but the green ramp does
not, so both are restated under html[data-theme='dark'], including the
hover overrides - without those the light border wins on specificity when a
recorded vote is hovered in dark mode.
Verified against a running dev server rather than on paper: the resolved
colours on the live buttons match the values a real .theme-admonition-tip
and .theme-admonition-warning compute to in the same page, in both themes,
and all four labels clear AA - 9.19:1 and 15.73:1 light, 7.76:1 and
10.35:1 dark.
Review of d3795ba found the one thing a verbatim copy could not survive. An admonition's soft border encloses a full-width tinted block, so it only has to be a boundary; on a small toggle it is what says which button you pressed. The fills are 1.06:1 light and 1.01 to 1.02:1 dark against the page - effectively invisible - so the border is the whole indicator, and copied across it measured 1.23:1 light and 1.36:1 dark for green, 1.33 and 1.09 for yellow. That is under the 3:1 WCAG 1.4.11 asks of a state indicator, and in light it was fainter than an untouched button's own border, so pressing a vote dimmed its outline. Each state now takes the saturated step its own admonition already uses - the tip icon, the warning inline code - so every value still comes from the pair this widget is modelled on: vs page vs fill .on --light-green-80 #079669 3.77 3.56 --dark-green-60 #90b9ae 7.70 7.76 .bad --light-yellow-120 #4d3800 11.16 10.51 --dark-brown-40 #d6c8b4 10.10 10.35 Dark figures are against --ifm-background-color #141f2e (custom.scss:133), which is what .box paints, not Infima's stock #1b1b1d. Be clear about what this is: a departure. Neither ramp has a saturated border anywhere else on the site - these four tokens appear only as icon and text colours - so this is a new recipe assembled from the two palettes rather than a copy of an existing one. It is the smallest departure available, one value per state, because the instruction to match the callouts and the requirement that a pressed state be visible cannot both be met literally: the callout border was drawn for a full-width block and does not transfer to a 90px toggle. Yellow has no mid step that carries on white - --light-yellow-100 reaches only 1.66:1 - so .bad's light border reads heavier than .on's green. The ramps are not symmetrical. --light-green-100 #064e3b would even them up at 9.72:1 and is already .on's own label token; #079669 was chosen instead to keep the outline light. Worth revisiting on the rendered page. Before settling on this I surveyed the site for a closer analogue than a callout - a control with a selected state. There are several, and none is a better model. The framework selectors mark selection by filling the chip (FrameworkSelectorMobile .checkedFramework, FrameworkCard .input:checked, FrameworksMobile, CardAdditional) but do it with one-off values that sit outside the brand ramps - #121619, #0f1723, white, or --frameworksBg which resolves to #121619 / #f0f0f0 - which is the objection this PR exists to fix, and they are monochrome, so they cannot tell a helpful vote from an unhelpful one. The active sidebar entry (menu.scss:27) uses --light-blue-60 behind --light-blue-100, the active TOC entry (toc.scss:41) uses that foreground alone, and SkillsCallout .tabActive recolours text only; all three are semantically "you are here", not "this is your answer". Separately, that sidebar pairing computes to 3.80:1 in dark, under AA for text - a pre-existing issue worth raising on its own, not a reason against blue here, since a border would only need 3:1. The recorded-vote hover overrides carry the same steps, or hovering a recorded vote would drop it back under the bar. Fill and label are untouched, so the four label ratios from the last commit still stand: 9.19 and 15.73 light, 7.76 and 10.35 dark. .failed keeps the soft border. It is a block notice, which is the case the admonition recipe was drawn for, and it now reads distinctly from a recorded .bad rather than identically. Also from review: restate .vote:hover for dark, since the dark state rules outranked it and a held-but-unrecorded vote - which is every vote until the capture returns - kept its state border on hover in dark while turning primary in light. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
moritzhartmeier
approved these changes
Sep 15, 2026
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
2 participants
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.
No description provided.