fix(ui): use DS::Tooltip and DS::Pill for the insights nav badge#2805
fix(ui): use DS::Tooltip and DS::Pill for the insights nav badge#2805tryeverything24 wants to merge 1 commit into
Conversation
The insights nav icon hand-rolled both of its affordances: a raw title= attribute as the tooltip and a rounded-full span as the unread-count badge. Route both through the design system: DS::Tooltip (as: :span, since the trigger sits inside the already-focusable link) replaces the title= attribute, and DS::Pill (filled neutral badge) renders the unread count. The link keeps its aria-label, so the accessible name is unchanged. Addresses Finding 2 of the DS Drift Patrol report in we-promise#2745.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe desktop Insights navigation control now wraps its lightbulb icon with ChangesInsights navigation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 961b389846
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| <%# `as: :span` because the trigger sits inside this already- | ||
| focusable link — the tooltip replaces the raw title= | ||
| attribute; the link keeps its aria-label. %> | ||
| <%= render DS::Tooltip.new(text: t("layouts.application.insights"), icon: "lightbulb", size: "sm", as: :span) %> |
There was a problem hiding this comment.
When the pointer is over the link's padding—or the link receives keyboard focus—this tooltip never opens because DS::Tooltip listens on its own descendant span; focus events do not propagate down from the <a>, and its controller only binds ancestor listeners for <summary> elements (app/components/DS/tooltip_controller.js:29-54). Consequently, replacing the link's title limits the label to hovering directly over the small lightbulb glyph instead of the full 36/44px hit target and provides no focus-triggered tooltip. The tooltip trigger/controller needs to encompass or explicitly bind to the link.
Useful? React with 👍 / 👎.
|
Reviewed the diff against the DS component APIs ( Likely bug: keyboard focus won't reveal the new tooltip
That guarantee doesn't actually hold here. this.summaryAncestor = this.element.closest("summary");
if (this.summaryAncestor) {
this.summaryAncestor.addEventListener("focusin", this.show);
...
}The Insights trigger's ancestor is an Worth confirming with a manual keyboard pass before merge. Fix would likely be generalizing the controller's ancestor lookup (e.g. Everything else looks clean:
Generated by Claude Code |
2 participants
PR body — we-promise/sure — account: tryeverything24 (PR #2 on this repo)
Branch:
fix/ds-tooltip-pill-insights-nav-badge(in WSL clone/root/sure, commit961b3898, based onorigin/main@6b6ae0ca)Title:
fix(ds): use DS::Tooltip and DS::Pill for the insights nav badgeSummary
Addresses Finding 2 of the DS Drift Patrol report in #2745.
The insights nav icon (added in #2550,
app/views/layouts/application.html.erb) hand-rolled both of its affordances:title:attribute on the icon-only link as its tooltip, androunded-fullspan with fixed sizing as the unread-count badge.What changed
title=is replaced byDS::Tooltipwithas: :span— the component's documented mode for triggers inside an already-focusable interactive ancestor (here, the nav link). Same conversion the repo already merged for the accounts eye-off indicator. The link keeps itsaria: { label: }, so the accessible name is unchanged.DS::Pill(badge mode,style: :filled,:neutraltone → dark chip with white label, closest DS equivalent to the previousbg-inverse/text-inverse). Positioned via an absolute wrapper span at the icon corner, following thelayouts/shared/_nav_item.html.erbprecedent (which wraps its previewDS::Pillthe same way); nudged to-top-1 -right-1so the DS-scale pill doesn't cover the lightbulb.unread_insights_count.positive?guard are untouched; the turbo-prefetch opt-out and its explanatory comment are untouched.Notes for review
title=(native titles never showed on focus either; screen readers get the retainedaria-label).layouts.application.insights.Test plan
bin/rails test— full suite green (5,920 runs, 23,808 assertions, 0 failures, 0 errors).bin/rails test test/controllers/insights_controller_test.rb— green (covers the layout's unread-badge path).bundle exec erb_lint app/views/layouts/application.html.erb— no offenses.bin/rubocop— clean;bin/brakeman— no warnings.Checklist for opening: base =
main, allow edits from maintainers ON. Do NOT use closing keywords (#2745's Finding 1 is covered separately) — reference it as "Addresses Finding 2 of #2745".Summary by CodeRabbit
9+for larger totals.