fix: download correct variant and fix layout shift on brand page #2595
Merged
ghostdevv merged 4 commits intonpmx-dev:mainfrom Apr 20, 2026
Merged
fix: download correct variant and fix layout shift on brand page #2595ghostdevv merged 4 commits intonpmx-dev:mainfrom
ghostdevv merged 4 commits intonpmx-dev:mainfrom
Conversation
- Fix on-light PNG downloading the on-dark asset (handler always used logo.src) - Fix Customize PNG export blocked by CSP — use data URL instead of blob URL - Render BrandCustomize in SSR (remove ClientOnly); canvas/download only runs on click Closes npmx-dev#2565 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Contributor
📝 WalkthroughWalkthroughAdds a Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Adebesin-Cell
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Resolves html-validate errors that surfaced once BrandCustomize began rendering in SSR: replace ButtonBase with role="radio" with native <input type="radio"> in labels, and drop class duplicates already provided by ButtonBase. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/Brand/Customize.vue`:
- Around line 147-167: The visible swatch selection (computed/prop
activeAccentId) can be out of sync with the native radio because customAccent
starts null and the aria-label is on the label; update the radio input binding
so the native control always reflects the visible state: either initialize
customAccent to activeAccentId (or set customAccent when activeAccentId changes)
and/or bind the input's checked state to compare customAccent === color.id, and
move the accessible name (aria-label / aria-labelledby) from the label onto the
input element so assistive tech reads the radio itself; adjust code locations
using pickerColors, activeAccentId, and customAccent to implement this.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 83f269f4-c11b-4b1b-bb40-eed1a84a6315
📒 Files selected for processing (1)
app/components/Brand/Customize.vue
…lasses" This reverts commit 6332984.
Replace ButtonBase with role="radio" with native <input type="radio"> in labels to satisfy html-validate prefer-native-element, and drop duplicate utility classes already provided by ButtonBase. Add focus-within rings for keyboard visibility. Bind :checked from activeAccentId so the native radio matches the visible swatch even when customAccent is null. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/components/Brand/Customize.vue (1)
174-207: Group the background radios with<fieldset>/<legend>for parity with the accent picker.The two on-dark/on-light radios share
name="brand-customize-bg"but aren’t wrapped in a<fieldset>, and the visible<span>at Lines 176-178 isn’t programmatically associated with the group. The accent picker just above uses<fieldset>+<legend class="sr-only">; mirroring that here keeps the a11y model consistent and lets screen readers announce the group name when focus enters the radios.As per coding guidelines (
**/*.vue: Composition API is already used; this is a template-semantics nit, not a refactor of reactivity).♿ Suggested fix
- <div class="flex items-center gap-3"> - <span class="text-sm font-mono text-fg-muted">{{ - $t('brand.customize.bg_label') - }}</span> - <div class="flex items-center border border-border rounded-md overflow-hidden"> + <fieldset class="flex items-center gap-3 border-none p-0 m-0"> + <legend class="text-sm font-mono text-fg-muted float-none contents"> + {{ $t('brand.customize.bg_label') }} + </legend> + <div class="flex items-center border border-border rounded-md overflow-hidden"> <label ...>...</label> <label ...>...</label> - </div> - </div> + </div> + </fieldset>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/Brand/Customize.vue` around lines 174 - 207, Wrap the two background radio inputs (the elements using v-model="customBgDark" and name="brand-customize-bg") in a <fieldset> and add a <legend class="sr-only"> that contains {{$t('brand.customize.bg_label')}} to match the accent picker pattern; also mark the visible span that currently shows $t('brand.customize.bg_label') as aria-hidden (or remove it) to avoid duplicate announcements so screen readers use the legend for the group label.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/components/Brand/Customize.vue`:
- Around line 174-207: Wrap the two background radio inputs (the elements using
v-model="customBgDark" and name="brand-customize-bg") in a <fieldset> and add a
<legend class="sr-only"> that contains {{$t('brand.customize.bg_label')}} to
match the accent picker pattern; also mark the visible span that currently shows
$t('brand.customize.bg_label') as aria-hidden (or remove it) to avoid duplicate
announcements so screen readers use the legend for the group label.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b0e41369-1589-44a6-9c03-ae0f1d065b39
📒 Files selected for processing (1)
app/components/Brand/Customize.vue
Contributor
Author
|
@ghostdevv Feel free to check it out! |
ghostdevv
approved these changes
Apr 20, 2026
ghostdevv
changed the title
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.
Summary
/brandreturning the on-dark asset — the click handler always usedlogo.srcregardless of which button was pressedblob:URL (not allowed byimg-src) for adata:URL<ClientOnly>aroundBrandCustomizeso the section renders during SSR (no layout gap / hydration flash); canvas + download logic only runs in click handlers, so it's safe on the serverCloses #2565
Test plan
/brand→ wordmark on-light → Download PNG → file is the light variant/brand→ logo mark on-light → Download PNG → file is the light variant