feat: draft UI for prerequisite summary#4401
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4401 +/- ##
==========================================
+ Coverage 54.52% 56.48% +1.95%
==========================================
Files 274 318 +44
Lines 6076 6970 +894
Branches 1455 1684 +229
==========================================
+ Hits 3313 3937 +624
- Misses 2763 3033 +270 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@greptile |
Confidence Score: 4/5Safe to merge; changes are additive UI-only with no data mutations or API calls. The implementation is straightforward and well-tested for its two primary branches. The only concern is the removal of the focus outline on the trigger button without a visible replacement, which could affect keyboard accessibility. Everything else — the CSS variable extraction, the guard logic in the component, and the layout change — looks correct. website/src/views/components/module-info/RequisiteRulePopup.scss — the focus style on the trigger button.
|
| Filename | Overview |
|---|---|
| website/src/views/components/module-info/RequisiteRulePopup.tsx | New component that conditionally renders an info icon tooltip showing the raw prerequisite rule; logic is clean and defensive. |
| website/src/views/components/module-info/RequisiteRulePopup.scss | Styles for the popup trigger and content; outline: none on focus removes keyboard focus indicator without a replacement. |
| website/src/styles/utils/css-variables.scss | Adds --tooltip-bg and --tooltip-color CSS variables for both light and dark modes, replacing the repurposed --gray/--body-bg usage in tippy.css. |
| website/src/styles/tippy/tippy.css | Arrow and background colors updated to use the new dedicated --tooltip-bg / --tooltip-color CSS variables; straightforward rename. |
| website/src/views/modules/ModulePageContent.tsx | Wraps the "Prerequisite Tree" heading in a flex row and conditionally renders RequisiteRulePopup when prerequisiteRule is present. |
| website/src/views/modules/ModulePageContent.scss | Adds sectionHeadingRow flex container that properly nests and overrides the existing sectionHeading styles. |
| website/src/views/components/module-info/RequisiteRulePopup.test.tsx | Tests cover the two key render branches: no-op when rule equals summary, and info button rendered when they differ. |
| website/src/views/modules/ModuleTree.tsx | Minor wording change in the beta-warning banner from "prerequisite text" to "prerequisite information" to match the new UI. |
| website/src/views/modules/snapshots/ModuleTree.test.tsx.snap | Snapshot updated to reflect the wording change in the warning banner. |
Sequence Diagram
sequenceDiagram
participant User
participant ModulePageContent
participant RequisiteRulePopup
participant Tooltip
participant LinkModuleCodes
ModulePageContent->>ModulePageContent: Check module.prerequisiteRule
alt prerequisiteRule exists
ModulePageContent->>RequisiteRulePopup: rule, summary
RequisiteRulePopup->>RequisiteRulePopup: rule.trim() empty? return null
RequisiteRulePopup->>RequisiteRulePopup: "summary === rule? return null"
RequisiteRulePopup->>Tooltip: Render info button trigger
User->>Tooltip: Hover / hold (touch)
Tooltip->>LinkModuleCodes: Render rule string
LinkModuleCodes->>User: Rule text with clickable module code links
else no prerequisiteRule
ModulePageContent->>ModulePageContent: Render heading only
end
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
website/src/views/components/module-info/RequisiteRulePopup.scss:19-23
**Focus outline removed without replacement**
`outline: none` on `:focus` discards the browser's default focus ring for keyboard users. The icon color change alone (from `--gray-light` to `theme-color()`) is unlikely to meet WCAG 2.1 SC 2.4.11's visible focus requirement, and many custom themes may have low contrast between these two values. Consider replacing with a visible focus-visible outline (e.g., `outline: 2px solid currentColor`) instead of suppressing it.
Reviews (1): Last reviewed commit: "Merge branch 'master' into feat/support-..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| &:hover, | ||
| &:focus { | ||
| outline: none; | ||
| color: theme-color(); | ||
| } |
There was a problem hiding this comment.
outline: none on :focus discards the browser's default focus ring for keyboard users. The icon color change alone (from --gray-light to theme-color()) is unlikely to meet WCAG 2.1 SC 2.4.11's visible focus requirement, and many custom themes may have low contrast between these two values. Consider replacing with a visible focus-visible outline (e.g., outline: 2px solid currentColor) instead of suppressing it.
Prompt To Fix With AI
This is a comment left during a code review.
Path: website/src/views/components/module-info/RequisiteRulePopup.scss
Line: 19-23
Comment:
**Focus outline removed without replacement**
`outline: none` on `:focus` discards the browser's default focus ring for keyboard users. The icon color change alone (from `--gray-light` to `theme-color()`) is unlikely to meet WCAG 2.1 SC 2.4.11's visible focus requirement, and many custom themes may have low contrast between these two values. Consider replacing with a visible focus-visible outline (e.g., `outline: 2px solid currentColor`) instead of suppressing it.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed in 3a74cc1 — split hover/focus styles and added a :focus-visible outline (2px solid currentColor with offset) while keeping outline: none only for :focus:not(:focus-visible).
Confidence Score: 4/5Safe to merge; the new popup is an additive UI element and the CSS variable refactor corrects an existing dark-mode bug rather than introducing new regressions. The only non-trivial concern is that the trigger button suppresses the browser focus ring entirely without a keyboard-accessible replacement, which could affect users navigating by keyboard. Everything else — the early-return guards, tooltip content rendering, CSS variable scoping, and test coverage — looks correct. website/src/views/components/module-info/RequisiteRulePopup.scss — the
|
| Filename | Overview |
|---|---|
| website/src/views/components/module-info/RequisiteRulePopup.tsx | New component that shows the raw structured prerequisite rule in an interactive Tippy tooltip when it differs from the human-readable summary. Logic is clean; early-return guards handle empty and identical strings correctly. |
| website/src/views/components/module-info/RequisiteRulePopup.scss | Styles for the popup trigger and content. Focus outline is removed without a visible replacement, which is a keyboard accessibility concern. |
| website/src/styles/utils/css-variables.scss | Adds --tooltip-bg and --tooltip-color CSS variables for both light and dark modes, fixing a pre-existing dark mode bug where tooltip text was dark-on-dark. |
| website/src/styles/tippy/tippy.css | Migrates tooltip arrow and background references from --gray / --body-bg to the new --tooltip-bg / --tooltip-color variables, correcting dark-mode rendering. |
| website/src/views/modules/ModulePageContent.tsx | Wraps the 'Prerequisite Tree' heading in a flex row and conditionally renders the new RequisiteRulePopup when module.prerequisiteRule is present. |
Sequence Diagram
sequenceDiagram
participant User
participant ModulePageContent
participant RequisiteRulePopup
participant Tooltip
participant LinkModuleCodes
ModulePageContent->>ModulePageContent: render Prerequisite Tree section
alt module.prerequisiteRule is set
ModulePageContent->>RequisiteRulePopup: rule, summary
alt "rule.trim() is empty OR summary === rule"
RequisiteRulePopup-->>ModulePageContent: null
else rule differs from summary
RequisiteRulePopup-->>ModulePageContent: Info icon button
User->>RequisiteRulePopup: hover / focus / hold
RequisiteRulePopup->>Tooltip: show content
Tooltip->>LinkModuleCodes: render rule string
LinkModuleCodes-->>User: rule text with clickable module code links
end
end
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
website/src/views/components/module-info/RequisiteRulePopup.scss:19-23
**Focus outline removed without replacement**
`outline: none` on `:focus` suppresses the browser's default focus ring for keyboard users without a visible alternative. The color shift to `theme-color()` on a small icon button is unlikely to meet WCAG 2.4.7 Focus Visible. Consider using `outline: none` only on `:focus:not(:focus-visible)` and keeping the ring for `:focus-visible`, or adding an explicit `box-shadow`/`outline` replacement to keep keyboard navigation accessible.
Reviews (2): Last reviewed commit: "Merge branch 'master' into feat/support-..." | Re-trigger Greptile
| &:hover, | ||
| &:focus { | ||
| outline: none; | ||
| color: theme-color(); | ||
| } |
There was a problem hiding this comment.
outline: none on :focus suppresses the browser's default focus ring for keyboard users without a visible alternative. The color shift to theme-color() on a small icon button is unlikely to meet WCAG 2.4.7 Focus Visible. Consider using outline: none only on :focus:not(:focus-visible) and keeping the ring for :focus-visible, or adding an explicit box-shadow/outline replacement to keep keyboard navigation accessible.
Prompt To Fix With AI
This is a comment left during a code review.
Path: website/src/views/components/module-info/RequisiteRulePopup.scss
Line: 19-23
Comment:
**Focus outline removed without replacement**
`outline: none` on `:focus` suppresses the browser's default focus ring for keyboard users without a visible alternative. The color shift to `theme-color()` on a small icon button is unlikely to meet WCAG 2.4.7 Focus Visible. Consider using `outline: none` only on `:focus:not(:focus-visible)` and keeping the ring for `:focus-visible`, or adding an explicit `box-shadow`/`outline` replacement to keep keyboard navigation accessible.
How can I resolve this? If you propose a fix, please make it concise.
Use :focus-visible with a visible outline so keyboard users can see focus while mouse clicks still suppress the default ring. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Closes #4301