feat(a11y): add reusable Alert component#1955
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ShroXd
changed the title
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new Vue 3 Single-File Component at app/components/Alert.vue with typed props: required Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/nuxt/a11y.spec.ts (1)
3515-3542: Please cover theme-specific contrast here as well.These cases only exercise the default theme, but the component exists to fix alert contrast across light/dark and background-theme combinations. Adding
Alertto the pooled theme matrix will catch the exact regression this PR is meant to prevent.Example addition to the pooled theme suite
const components = [ + { + name: 'AlertWarning', + mount: () => + mountSuspended(Alert, { + props: { variant: 'warning', title: 'Warning title' }, + slots: { default: 'Warning body' }, + }), + }, + { + name: 'AlertError', + mount: () => + mountSuspended(Alert, { + props: { variant: 'error', title: 'Error title' }, + slots: { default: 'Error body' }, + }), + }, { name: 'AppHeader', mount: () => mountSuspended(AppHeader) }, { name: 'AppFooter', mount: () => mountSuspended(AppFooter) },
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d9d44df0-631a-4bde-8b8c-83b1988f2928
📒 Files selected for processing (3)
app/components/Alert.vueapp/components/Package/ClaimPackageModal.vuetest/nuxt/a11y.spec.ts
| > | ||
| {{ mergedError }} | ||
| </div> | ||
| <Alert v-if="mergedError" variant="error">{{ mergedError }}</Alert> |
There was a problem hiding this comment.
This publish-error alert never renders.
mergedError is computed as null whenever checkResult exists, so once the availability check has succeeded this branch cannot show failures from handleClaim(). If publishing fails, the modal stays open without surfacing the error. Please separate availability-check errors from publish errors, or let publishError win before the checkResult !== null guard.
One way to untangle the two error paths
-const mergedError = computed(() => {
- return checkResult.value !== null
- ? null
- : (publishError.value ??
- (checkError.value instanceof Error
- ? checkError.value.message
- : $t('claim.modal.failed_to_check')))
-})
+const availabilityError = computed(() => {
+ if (checkResult.value !== null) return null
+ return checkError.value instanceof Error
+ ? checkError.value.message
+ : $t('claim.modal.failed_to_check')
+})
+
+const mergedError = computed(() => {
+ return publishError.value ?? availabilityError.value
+})There was a problem hiding this comment.
This is existing logic, since this PR only focuses on the UI part, ignore it for now.
There was a problem hiding this comment.
@ShroXd, understood — that logic predates this PR and is outside its scope. I'll leave it for a future fix. The UI refactoring with the new Alert component looks clean and the before/after screenshots show a clear improvement in contrast for both light and dark themes.
🧠 Learnings used
Learnt from: alexdln
Repo: npmx-dev/npmx.dev PR: 838
File: app/pages/package/[...package].vue:445-449
Timestamp: 2026-02-03T13:59:33.392Z
Learning: The copy button pattern in app/pages/package/[...package].vue may be made into a reusable component or pattern in the future, but currently it's acceptable to keep it inline with the CSS-only approach for smooth animations.
Learnt from: jellydeck
Repo: npmx-dev/npmx.dev PR: 904
File: app/components/Package/AccessControls.vue:253-253
Timestamp: 2026-02-04T05:34:20.527Z
Learning: In the npmx.dev project, ensure that focus-visible styling for button and select elements is implemented globally in app/assets/main.css with the rule: button:focus-visible, select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }. Do not apply per-element inline utility classes like focus-visible:outline-accent/70 on these elements in Vue templates or components (e.g., AccessControls.vue). Rely on the global rule for consistency and maintainability; only use inline focus-visible utilities when styling non-button/select elements or in exceptional cases outside the global scope.
Learnt from: jellydeck
Repo: npmx-dev/npmx.dev PR: 904
File: app/components/Package/Versions.vue:332-332
Timestamp: 2026-02-04T05:34:54.335Z
Learning: In the npmx.dev project, focus-visible styling for buttons and selects is applied globally via main.css with: button:focus-visible, select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }. Therefore, individual buttons or selects in Vue components should not rely on inline focus-visible utility classes like focus-visible:outline-accent/70. Ensure components do not add per-element focus-visible utilities; rely on the global rule. If a component seems to require a different focus state, adjust only through global CSS or maintain accessibility with native focus styles, rather than inserting inline utility classes.
Learnt from: userquin
Repo: npmx-dev/npmx.dev PR: 1170
File: app/components/Package/MetricsBadges.vue:66-66
Timestamp: 2026-02-08T15:02:02.232Z
Learning: In Vue components that use UnoCSS with the preset-icons collection, prefer colon-syntax for icons (e.g., i-carbon:checkmark) over the dash-separated form (i-carbon-checkmark). This aids UnoCSS in resolving the correct collection directly, which improves performance for long icon names. Apply this pattern to all Vue components (e.g., app/components/**/*.vue) where UnoCSS icons are used; ensure UnoCSS is configured with the preset-icons collection.
Learnt from: userquin
Repo: npmx-dev/npmx.dev PR: 1335
File: app/components/Compare/FacetSelector.vue:72-78
Timestamp: 2026-02-10T15:47:33.467Z
Learning: In the npmx.dev project, ButtonBase (used via app/components/ButtonBase.vue or similar) provides default classes: border border-border. When styling ButtonBase instances in Vue components (e.g., app/components/Compare/FacetSelector.vue and other files under app/components), avoid duplicating the border class to prevent the HTML validator error no-dup-class and CI failures. If styling overrides are needed, ensure only unique classes are applied (remove redundant border classes or adjust via props) so the default border remains intact without duplication.
Learnt from: abbeyperini
Repo: npmx-dev/npmx.dev PR: 1049
File: app/components/Settings/Toggle.client.vue:22-29
Timestamp: 2026-02-11T00:01:33.121Z
Learning: In Vue 3.4 and later, you can use same-name shorthand for attribute bindings: use :attributeName instead of :attributeName="attributeName" when binding to a variable with the same name in scope. For example, :id is equivalent to :id="id" when an id variable exists. Apply this shorthand in .vue components (notably in Settings/Toggle.client.vue) to simplify templates. Ensure the bound variable exists and that you are using a Vue version that supports this shorthand.
Learnt from: alexdln
Repo: npmx-dev/npmx.dev PR: 1845
File: app/components/InstantSearch.vue:6-11
Timestamp: 2026-03-03T09:42:52.533Z
Learning: Maintain the established prehydration pattern across the project: use JSON.parse(localStorage.getItem('npmx-settings') || '{}') without per-call try-catch blocks. Do not introduce try-catch error handling for this pattern unless a coordinated, project-wide refactor of all onPrehydrate readers is planned and executed.
ShroXd
force-pushed
the
create-alert-component
branch
from
5b04166 to
d89b38b
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/nuxt/a11y.spec.ts (1)
3515-3542: Assert the live-region role in these newAlerttests.These checks only prove the markup is axe-clean. They will not catch a regression where
warningstops renderingrole="status"orerrorstops renderingrole="alert", which is the key a11y contract of this component.Suggested test tightening
describe('Alert', () => { it('should have no accessibility violations for warning variant', async () => { const component = await mountSuspended(Alert, { props: { variant: 'warning', title: 'Warning title' }, slots: { default: 'This is a warning message.' }, }) + expect(component.attributes('role')).toBe('status') const results = await runAxe(component) expect(results.violations).toEqual([]) }) it('should have no accessibility violations for error variant', async () => { const component = await mountSuspended(Alert, { props: { variant: 'error', title: 'Error title' }, slots: { default: 'This is an error message.' }, }) + expect(component.attributes('role')).toBe('alert') const results = await runAxe(component) expect(results.violations).toEqual([]) }) it('should have no accessibility violations without title', async () => { const component = await mountSuspended(Alert, { props: { variant: 'warning' }, slots: { default: 'This is a warning message.' }, }) + expect(component.attributes('role')).toBe('status') const results = await runAxe(component) expect(results.violations).toEqual([]) }) })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9b16334e-ece5-40b1-8fc8-dbf83bb2eadf
📒 Files selected for processing (3)
app/components/Alert.vueapp/components/Package/ClaimPackageModal.vuetest/nuxt/a11y.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/Alert.vue
1 participant
🔗 Linked issue
Resolves #1872
🧭 Context
As the issue mentioned, there are two aspects that need to be improved. First is the low contrast of the warning alert, second is there are many different alert stylings in the repo, it's worth creating a common one.
📚 Description
I checked the existing design pattern in the npmx and built a common Alert component. It supports warning and error variants for now, and is currently only used in the claim package name modal. Once maintainers and the community think it's good enough, I'll create another PR to replace the remaining alert UI across the website.
Modal with warning message
Modal with validation warning and error
I didn't find an easy way to trigger these scenarios in the UI or browser, so I hardcoded some messages in the code to verify the appearance. Since this PR only includes UI changes, I think this approach is acceptable. Please point it out if I missed something.