Skip to content

Add "Was this helpful?" feedback prompt to doc pages#412

Open
vvlladd28 wants to merge 3 commits into
mainfrom
feedback-prompt
Open

Add "Was this helpful?" feedback prompt to doc pages#412
vvlladd28 wants to merge 3 commits into
mainfrom
feedback-prompt

Conversation

@vvlladd28
Copy link
Copy Markdown
Member

Summary

  • New FeedbackPrompt.astro component posts feedback (yes/no, reason, free text, page path) to Formspree form mjgzrgwn as JSON.
  • Mounted in the right sidebar under the TOC on pages that have one; falls back inline at the bottom of MarkdownContent on TOC-less doc pages (mobile-only when TOC is present, so the two never both render).
  • Hidden on splash pages (no hasSidebar) and on landing/overview pages that use a hero: block in frontmatter — those aggregate links and have no actionable content of their own. Rule covers all products (CE / PE / PaaS / Edge / Trendz / IoT Gateway / TBMQ / Mobile / License Server) automatically via the shared hero: pattern.
  • Vanilla JS state machine inside a <script> block (no React/Turnstile); honeypot _gotcha ships with the form, spam protection beyond that is configured in the Formspree dashboard.

Posts feedback (yes/no, reason, free-text, page path) to Formspree
form mjgzrgwn as JSON. Mounted in the right sidebar on pages with a
TOC and inline at the bottom of MarkdownContent on TOC-less doc pages
(mobile-only fallback when TOC is present). Hidden on splash pages
and on landing/overview pages with a hero block in frontmatter, since
those aggregate links rather than carry actionable content.
Copy link
Copy Markdown
Member Author

@vvlladd28 vvlladd28 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review summary

Reviewed 3 changed files in Add "Was this helpful?" feedback prompt to doc pages. Left 6 comment(s) inline.

The component is well-structured and accessible at the markup level (real radios, real form, label associations, honeypot, not-content to escape Starlight typography). Most findings are around the submission path (fetch headers + how failures are surfaced) and a few accessibility/UX gaps in the state transitions. The dual-mount design (sidebar + inline) is clean from a layout standpoint but leaves the two instances completely independent, which can produce duplicate submissions in an edge case.


This is an auto-generated review. Findings may contain errors — please verify before applying changes.

try {
await fetch(endpoint, {
method: 'POST',
headers: { Accept: 'application/json' },
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The body is a JSON string but the request doesn't include Content-Type: application/json. Without it, fetch defaults to text/plain;charset=UTF-8, and Formspree's parser may not pick the JSON path — submissions could land with info/reason/option/page either dropped or lumped into a single string field on the dashboard. Worth setting 'Content-Type': 'application/json' alongside Accept, or switching the body to new FormData(form) (with option and page appended) so Formspree gets the multipart form encoding it natively understands.

});
} catch {
/* Fail silently — Formspree may be unreachable; user has moved on. */
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fetch only rejects on network errors — a 4xx or 5xx from Formspree resolves successfully, so the catch block here is bypassed and the success state is shown unconditionally below. Combined with the missing Content-Type header in the call above, a 400 from Formspree would silently look like a happy submission to the user. Consider checking response.ok and at minimum not transitioning to the success state on failure (or surfacing a discreet "couldn't send — try again" message), since the only spam protection in front of this is the honeypot.

form.hidden = false;
root.querySelectorAll<HTMLElement>('[data-option-fieldset]').forEach((fs) => {
fs.hidden = fs.dataset.optionFieldset !== chosen;
});
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the buttons → form transition happens, keyboard and screen-reader users get no signal that a new control region appeared — focus is left on the now-hidden Yes/No button. Moving focus to the first radio in the revealed fieldset (e.g. root.querySelector<HTMLInputElement>([data-option-fieldset="${chosen}"] .feedback-reason-input)?.focus()) would make this work for keyboard users. Same applies to the success transition further down — nothing announces "Thank you for your feedback!" to assistive tech.

<button type="submit" class="feedback-submit" disabled>Submit</button>
</form>

<div class="feedback-success" data-feedback-success hidden>
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This success message swaps in silently — assistive tech won't announce it. Adding role="status" (or aria-live="polite") on this element would let screen readers read out "Thank you for your feedback!" when it appears. The element being initially hidden doesn't prevent the live region announcement once hidden is removed.

}

const init = () =>
document.querySelectorAll<HTMLElement>('[data-feedback-root]').forEach(bind);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heads-up that on pages with a TOC both the sidebar and inline <FeedbackPrompt /> are rendered (only one is visible at a time via CSS), and bind() gives each its own independent chosen / submit state. If a user submits on desktop and then resizes to a narrow viewport (or vice-versa), the other instance is still in its initial state and accepts a second submission for the same page. A small sessionStorage/localStorage flag keyed by location.pathname (set after a successful submit, checked at bind() time to skip straight to the success view) would also handle the more common case of "user submits, navigates away, comes back".

no: [
['Hard to understand', 'hard-to-understand'],
['Incorrect information', 'incorrect-information'],
['Missing the information', 'missing-the-information'],
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor copy nit — "Missing the information" reads awkwardly next to the others. "Information is missing" or just "Missing information" would match the tone of the other options.

- Add Content-Type: application/json header so Formspree picks the JSON
  parser instead of falling back to text/plain
- Move focus to the first radio when the form opens so keyboard users
  get a clear next step after picking Yes/No
- Add role="status" to the success block so assistive tech announces
  "Thank you for your feedback!" when it appears
- Persist a per-pathname flag in sessionStorage + dispatch a custom
  event after submit, so the second (sidebar/inline) instance on the
  same page is kept in sync and revisits skip straight to success
- Rename reason "Missing the information" → "Missing information"
Copy link
Copy Markdown
Member Author

@vvlladd28 vvlladd28 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review summary

Reviewed 3 changed files in Add "Was this helpful?" feedback prompt to doc pages. Left 5 comment(s) inline.

The approach is clean — vanilla JS, no extra runtime deps, sessionStorage-keyed by pathname, honeypot for spam, and a responsive mount that avoids double-rendering at the breakpoint boundary. Main concerns: the hero/hasSidebar rule is only enforced in one of the two mount points (a latent trap rather than a current bug since every existing hero page sets tableOfContents: false), and the script accumulates document-level listeners across view-transition navigations. The rest are minor consistency and a11y nits.


This is an auto-generated review. Findings may contain errors — please verify before applying changes.

<div class="right-sidebar-panel sl-hidden lg:sl-block">
<div class="sl-container">
<TableOfContents />
<FeedbackPrompt />
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The showFeedback = hasSidebar && !entry.data.hero guard from MarkdownContent.astro isn't applied here — this mount renders whenever Astro.locals.starlightRoute.toc is truthy, regardless of hero: frontmatter or hasSidebar. The PR description says the widget should be hidden on splash pages and on landing/overview pages with a hero: block, but that rule lives only in MarkdownContent.astro.

In practice this isn't currently triggered because every existing hero: page in src/content/docs/** also sets tableOfContents: false, so the right-sidebar panel doesn't render and <FeedbackPrompt /> never gets a chance to mount. The hazard is purely latent — anyone who adds a future hero page without tableOfContents: false will see the widget appear in the right sidebar against the stated rule.

Would be more robust to compute showFeedback once (in a shared helper or by passing it in as a prop) and apply it at both mount points, instead of relying on the implicit "hero pages have TOC off" convention.

return;
}

document.addEventListener(SUBMITTED_EVENT, showSuccess);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

document.addEventListener(SUBMITTED_EVENT, showSuccess) is registered inside bind(), which runs on every astro:page-load. View Transitions fire astro:page-load on each navigation, the new feedback root gets bound, and a fresh listener is appended — but the previous listener is never removed. Its showSuccess closure holds references to the old title/buttons/form/success elements, so the detached DOM subtree from the prior page can't be garbage-collected.

Over a long browsing session with View Transitions, this accumulates. Cheaper fix: register the document listener once at module scope (outside bind) and have it iterate over current [data-feedback-root] elements via querySelectorAll, or use astro:before-swap to detach the per-root listener before navigation.

margin-top: 0.75rem;
padding: 0.625rem 0.75rem;
border-radius: 0.5rem;
background: #ecfdf5;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These success-state colors are hardcoded hex values (#ecfdf5, #047857, #6ee7b7) and a literal rgba(16, 185, 129, 0.12), while every other surface in this component uses var(--sl-color-*) tokens (--sl-color-bg, --sl-color-hairline, --sl-color-text-accent, …). Per CLAUDE.md, theme-dependent colors should go through CSS custom properties. Worth either reusing an existing accent token or defining a --feedback-success-{bg,fg} token alongside the rest so future theme tweaks stay in one place.

<button type="submit" class="feedback-submit" disabled>Submit</button>
</form>

<div class="feedback-success" data-feedback-success role="status" hidden>
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The live region starts with the hidden attribute and is unhidden later by JS. Some assistive tech inconsistently announces content that transitions from hidden (i.e. removed from the accessibility tree) to visible. The more reliable pattern is to keep the live region in the a11y tree throughout the lifecycle and update its text content when the submission completes — e.g. render the wrapper as aria-live="polite" always, leave it empty until submit, and then set textContent to the thank-you string.

page: location.pathname,
}),
});
} catch {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Submission failures (network error, 4xx/5xx from Formspree, honeypot caught by the spam filter) are swallowed and the user still sees the "Thank you for your feedback!" success state, plus sessionStorage records the page as submitted so they can't retry within the session. The trailing comment frames this as intentional ("analytics-only"), so feel free to ignore — but worth confirming you're OK with bots and offline users seeing the success affordance, and with genuine submission failures being silently dropped without even a console.warn. A minimal alternative: only call markSubmitted() when the response is response.ok, leave the form interactive otherwise.

- Hide feedback prompt on splash/hero landing pages
- Show success only on actual submission success (HTTP 200)
- Move SUBMITTED event listener to module scope to avoid View Transitions leaks
- Announce success via aria-live region to fix screen reader silence
- Replace hardcoded success colors with Starlight theme tokens
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant