Replace install platform dropdown with button group#1
Replace install platform dropdown with button group#1bschellenberger2600 wants to merge 1 commit into
Conversation
Use segmented platform buttons with shared session state, radiogroup semantics, and arrow-key navigation. Scope copy-button styles to actions so platform controls keep their own look. Made-with: Cursor
📝 WalkthroughWalkthroughThe changes replace a traditional HTML Changes
Sequence DiagramsequenceDiagram
actor User
participant Component as InstallPicker<br/>(Astro)
participant Script as install-pickers<br/>(JS Logic)
participant DOM as DOM/State
participant Style as CSS Styling
User->>Component: Page loads with platform picker
Component->>DOM: Render radiogroup with buttons
DOM->>Style: Apply default styles
Style-->>User: Display picker UI
User->>Script: Click platform button
Script->>DOM: Update aria-checked state
Script->>Script: Call syncPlatformButtons()
DOM->>Style: Reapply active state styles
Style-->>User: Highlight selected platform
User->>Script: Press Arrow/Home/End key
Script->>Script: Calculate new platform position
Script->>DOM: Update aria-checked & focus new button
Script->>Script: Call queueMicrotask for focus
DOM->>Style: Apply active styling to new button
Style-->>User: Visual feedback on selection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/styles/global.css (1)
510-520: Consider addingcursor: defaultto active platform buttons.Active buttons retain
cursor: pointerfrom the base style, which suggests interactivity even though re-clicking them has no meaningful effect. Settingcursor: defaultimproves the UX signal.♻️ Optional improvement
.install-picker[data-product='git-fire'] .install-picker__platform-btn--active { color: var(--color-fire); border-color: color-mix(in srgb, var(--color-fire) 45%, var(--color-border)); background: color-mix(in srgb, var(--color-fire) 12%, var(--color-bg)); + cursor: default; } .install-picker[data-product='git-rain'] .install-picker__platform-btn--active { color: var(--color-rain); border-color: color-mix(in srgb, var(--color-rain) 45%, var(--color-border)); background: color-mix(in srgb, var(--color-rain) 12%, var(--color-bg)); + cursor: default; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/styles/global.css` around lines 510 - 520, Add cursor: default to the active platform button rules so the .install-picker__platform-btn--active state no longer shows the pointer cursor from the base .install-picker__platform-btn styles; update both selectors (.install-picker[data-product='git-fire'] .install-picker__platform-btn--active and .install-picker[data-product='git-rain'] .install-picker__platform-btn--active) to include cursor: default to clearly signal non-interactivity when already active.src/scripts/install-pickers.ts (2)
29-37: Minor duplication withcoerceOslogic.The type guard in
storedOsduplicates whatcoerceOsalready provides. Consider reusing it for consistency.♻️ Optional refactor
function storedOs(): OsFamily | null { try { - const v = sessionStorage.getItem(STORAGE_KEY); - if (v === 'macos' || v === 'windows' || v === 'linux' || v === 'go') return v; + return coerceOs(sessionStorage.getItem(STORAGE_KEY) ?? undefined); } catch { /* private mode */ } return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scripts/install-pickers.ts` around lines 29 - 37, The storedOs() function duplicates the OS validation logic; replace the manual string checks by delegating to coerceOs: read sessionStorage.getItem(STORAGE_KEY) into a variable inside storedOs(), then pass that value to coerceOs(...) and return its result (or null) while preserving the try/catch for private mode; update references to storedOs accordingly and remove the redundant string checks so coerceOs becomes the single source of truth for OS coercion.
204-212: Picker initialization depends on copy button presence.The early return on line 206 means if
copyBtnis missing, the entire picker (including platform buttons) won't initialize. This is fine since the component always includes the copy button, but be aware of this coupling if the component structure changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scripts/install-pickers.ts` around lines 204 - 212, The early return when copyBtn is missing prevents initialization of the pickers (storedOs/detectOs, syncPlatformButtons, renderPicker, bindPlatformRadiogroup) because the code returns immediately if copyBtn is falsy; remove or move that early return so picker initialization runs regardless of the presence of copyBtn (i.e., ensure syncPlatformButtons(root, initial), renderPicker(root), and bindPlatformRadiogroup(root) execute with the detected initial value even when copyBtn is null) while still optionally guarding only copy-button-specific logic behind a copyBtn check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/scripts/install-pickers.ts`:
- Around line 29-37: The storedOs() function duplicates the OS validation logic;
replace the manual string checks by delegating to coerceOs: read
sessionStorage.getItem(STORAGE_KEY) into a variable inside storedOs(), then pass
that value to coerceOs(...) and return its result (or null) while preserving the
try/catch for private mode; update references to storedOs accordingly and remove
the redundant string checks so coerceOs becomes the single source of truth for
OS coercion.
- Around line 204-212: The early return when copyBtn is missing prevents
initialization of the pickers (storedOs/detectOs, syncPlatformButtons,
renderPicker, bindPlatformRadiogroup) because the code returns immediately if
copyBtn is falsy; remove or move that early return so picker initialization runs
regardless of the presence of copyBtn (i.e., ensure syncPlatformButtons(root,
initial), renderPicker(root), and bindPlatformRadiogroup(root) execute with the
detected initial value even when copyBtn is null) while still optionally
guarding only copy-button-specific logic behind a copyBtn check.
In `@src/styles/global.css`:
- Around line 510-520: Add cursor: default to the active platform button rules
so the .install-picker__platform-btn--active state no longer shows the pointer
cursor from the base .install-picker__platform-btn styles; update both selectors
(.install-picker[data-product='git-fire'] .install-picker__platform-btn--active
and .install-picker[data-product='git-rain']
.install-picker__platform-btn--active) to include cursor: default to clearly
signal non-interactivity when already active.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a01cb354-fc27-4030-bd6a-5c90ee51e7d0
📒 Files selected for processing (3)
src/components/InstallPicker.astrosrc/scripts/install-pickers.tssrc/styles/global.css
|
@cursoragent Ibelieve this branch is now obselete |
|
Here is what the repo shows. The comment is right: this branch is obsolete relative to
Your branch tip is still the older single commit PR status: PR #1 for this branch is already closed in GitHub, so there is nothing further you must do on the PR for “obsolete” to be handled. Optional cleanup: If you want the name gone from the remote, you can delete I did not change code or push commits: the correct action is to treat   |
1 participant
Summary
Replaces the platform
<select>inInstallPickerwith a segmented button radiogroup (macOS, Windows, Linux, Go) so the install surface matches the rest of the site’s control style.Changes
role="radiogroup"/role="radio", labeled viaaria-labelledby; platform label is ap.install-picker__labelinstead oflabel for.sessionStoragesync across pickers;aria-checked+ rovingtabindex; arrow keys and Home/End to move selection; focus stays on the active control after keyboard navigation.data-product; copy button styles scoped to.install-picker__actionsso platform buttons stay distinct.How to verify
npm run buildSummary by CodeRabbit
New Features
Style