feat: accessible rings, accent colors & visual changes#904
feat: accessible rings, accent colors & visual changes#904danielroe merged 21 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
jellydeck
changed the title
📝 WalkthroughWalkthroughRestructures accent colours into theme-aware Possibly related PRs
Suggested labels
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)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/Header/ConnectorModal.vue (1)
165-173:⚠️ Potential issue | 🟡 MinorAvoid cancelling the accent outline on inputs.
focus-visible:outline-nonesuppresses the accent outline you just added, so the focus indication falls back to the default ring. Consider removingoutline-none(and the ring if the outline is the intended cue).Proposed adjustment
- class="w-full px-3 py-2 font-mono text-sm bg-bg-subtle border border-border rounded-md text-fg placeholder:text-fg-subtle transition-colors duration-200 focus-visible:outline-none focus-visible:ring-2 focus-visible:outline-accent/70" + class="w-full px-3 py-2 font-mono text-sm bg-bg-subtle border border-border rounded-md text-fg placeholder:text-fg-subtle transition-colors duration-200 focus-visible:outline-accent/70"- class="w-full px-3 py-2 font-mono text-sm bg-bg-subtle border border-border rounded-md text-fg transition-colors duration-200 focus:border-border-hover focus-visible:outline-none focus-visible:ring-2 focus-visible:outline-accent/70" + class="w-full px-3 py-2 font-mono text-sm bg-bg-subtle border border-border rounded-md text-fg transition-colors duration-200 focus:border-border-hover focus-visible:outline-accent/70"Also applies to: 189-197
🧹 Nitpick comments (1)
app/components/Package/ListToolbar.vue (1)
184-205: Consider updating the sort direction toggle button's focus styling for consistency.The sort direction toggle button still uses ring-based focus styling (
focus-visible:ring-2 focus-visible:ring-fg focus-visible:ring-offset-2), whilst the rest of this PR standardises onfocus-visible:outline-accent/70. This creates a visual inconsistency.♻️ Suggested fix
<button v-if="!searchContext" type="button" - class="p-1.5 rounded border border-border bg-bg-subtle text-fg-muted hover:text-fg hover:border-border-hover transition-colors duration-200 focus-visible:ring-2 focus-visible:ring-fg focus-visible:ring-offset-2 focus-visible:ring-offset-bg" + class="p-1.5 rounded border border-border bg-bg-subtle text-fg-muted hover:text-fg hover:border-border-hover transition-colors duration-200 focus-visible:outline-accent/70" :aria-label="$t('filters.sort.toggle_direction')"
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/Package/InstallScripts.vue (1)
32-46:⚠️ Potential issue | 🟡 MinorAdd explicit outline width and offset to ensure visible focus indicators on both elements.
The global CSS in
app/assets/main.cssdefines outline width forbutton:focus-visibleanda:focus-visible, but the<dd>element on line 34 is not covered by those rules. Usingfocus-visible:outline-accent/70alone sets only the outline colour without explicit width or offset, which risks a missing or imperceptible focus indicator. For consistency and to ensure accessibility across browsers, add explicit outline utilities:Suggested fix
- class="font-mono text-sm text-fg-subtle m-0 truncate focus:whitespace-normal focus:overflow-visible cursor-help rounded focus-visible:outline-accent/70" + class="font-mono text-sm text-fg-subtle m-0 truncate focus:whitespace-normal focus:overflow-visible cursor-help rounded focus-visible:outline-2 focus-visible:outline-offset-2 focus-visible:outline-accent/70"- class="flex items-center gap-1.5 text-xs text-fg-muted hover:text-fg transition-colors duration-200 focus-visible:outline-accent/70 rounded" + class="flex items-center gap-1.5 text-xs text-fg-muted hover:text-fg transition-colors duration-200 focus-visible:outline-2 focus-visible:outline-offset-2 focus-visible:outline-accent/70 rounded"
jellydeck
force-pushed
the
visual-adjustments
branch
from
c3c9ae1 to
2ed643d
Compare
jellydeck
force-pushed
the
visual-adjustments
branch
from
2ed643d to
80a3876
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
app/components/Header/AuthModal.client.vue (1)
102-102:ring-offset-*classes have no effect when used withoutline.The
focus-visible:ring-offset-2andfocus-visible:ring-offset-bgutilities are designed to work with Tailwind'sringutilities (which usebox-shadow), not withoutline. These classes will be ignored since you're usingoutline-accent/70instead of a ring.For consistency with the disconnect button (line 37) and handle input (line 64), consider removing the ineffective
ring-offsetclasses:🧹 Proposed cleanup
<button type="submit" :disabled="!handleInput.trim()" - class="w-full px-4 py-2 font-mono text-sm text-bg bg-fg rounded-md transition-all duration-200 hover:bg-fg/90 disabled:opacity-50 disabled:cursor-not-allowed focus-visible:outline-accent/70 focus-visible:ring-offset-2 focus-visible:ring-offset-bg" + class="w-full px-4 py-2 font-mono text-sm text-bg bg-fg rounded-md transition-all duration-200 hover:bg-fg/90 disabled:opacity-50 disabled:cursor-not-allowed focus-visible:outline-accent/70" >Apply the same change to the create-account button (line 108) and Bluesky button (line 116).
Also applies to: 108-108, 116-116
app/components/Package/ListControls.vue (1)
78-86: Consider updating the select's focus styling for consistency.The search input above uses the new accent-based focus styling (
focus-visible:outline-accent/70), but the sort select still usesfocus:(border-border-hover outline-none). While the global CSS inmain.cssprovidesselect:focus-visiblestyling, the inlineoutline-nonemay override it.Consider updating the select to match the input's focus styling pattern for visual consistency within this component.
♻️ Suggested change
<select id="package-sort" v-model="sortValue" - class="appearance-none bg-bg-subtle border border-border rounded-lg ps-3 pe-8 py-2 font-mono text-sm text-fg cursor-pointer transition-colors duration-200 focus:(border-border-hover outline-none) hover:border-border-hover" + class="appearance-none bg-bg-subtle border border-border rounded-lg ps-3 pe-8 py-2 font-mono text-sm text-fg cursor-pointer transition-colors duration-200 focus:border-accent focus-visible:(outline-2 outline-accent/70) hover:border-border-hover" >app/components/Package/MetricsBadges.vue (1)
68-71: Consider adding explicit outline width.The focus styling uses
focus-visible:outline-accent/70which sets the colour, but unlike the previousring-2, there's no explicit outline width. Depending on the UnoCSS configuration, this may rely on browser defaults or inherited values.For consistency with other components in this PR (e.g.,
focus-visible:(outline-2 outline-accent/70)in ListControls.vue), consider adding an explicit width.♻️ Suggested change
typesHref - ? 'hover:text-fg hover:border-border-hover focus-visible:outline-accent/70' + ? 'hover:text-fg hover:border-border-hover focus-visible:(outline-2 outline-accent/70)' : '',app/components/Package/Versions.vue (1)
365-365: Consider using outline instead of text colour for focus indication.The version link uses
focus-visible:text-accentwhich only changes text colour on focus. This may be less perceivable than an outline, especially for users with colour vision deficiencies. Consider usingfocus-visible:outline-accent/70for consistency with other elements in this PR, or keep both for enhanced visibility.app/composables/useSettings.ts (1)
9-9: Type assumption relies on light/dark key parity.The type
AccentColorIdis derived solely fromACCENT_COLORS.light. This works correctly as long as bothlightanddarkobjects inACCENT_COLORShave identical keys. Consider adding a compile-time check or a comment to document this invariant, so future maintainers don't accidentally break it.app/utils/prehydrate.ts (1)
17-35: Hardcoded colour values must stay synchronised withACCENT_COLORS.The prehydrate callback is stringified, so hardcoding is necessary. However, this creates a maintenance burden: any change to
ACCENT_COLORSinshared/utils/constants.tsmust be manually mirrored here. Consider adding a comment referencing the source of truth, or a build-time/test-time check to detect drift.📝 Suggested comment
// Accent colors - hardcoded since ACCENT_COLORS can't be referenced + // IMPORTANT: Keep in sync with ACCENT_COLORS in shared/utils/constants.ts const colors = {
jellydeck
force-pushed
the
visual-adjustments
branch
from
e9aaa19 to
f2dc027
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/pages/compare.vue (1)
86-104:⚠️ Potential issue | 🟡 MinorAvoid per-button focus-visible utilities; use the global rule instead.
These buttons should inherit the global focus-visible outline. If underline-on-focus is still desired, move it to a global or shared CSS rule rather than inline utilities.
Suggested diff
- class="text-[10px] transition-colors focus-visible:outline-none focus-visible:underline focus-visible:underline-accent" + class="text-[10px] transition-colors" ... - class="text-[10px] transition-colors focus-visible:outline-none focus-visible:underline focus-visible:underline-accent" + class="text-[10px] transition-colors"Based on learnings: In the npmx.dev project, focus-visible styling for buttons and selects is applied globally via main.css using
button:focus-visible, select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }. Therefore, individual buttons or selects should not add per-element focus-visible utilities.
🧹 Nitpick comments (6)
app/components/Terminal/Install.vue (1)
124-131: Inlinefocus-visible:outline-accent/70is redundant on buttons.Per project conventions, focus-visible styling for buttons is applied globally via
main.css(button:focus-visible { outline: 2px solid var(--accent); ... }). Thefocus-visible:opacity-100is correctly needed to make the hidden button visible on keyboard focus, butfocus-visible:outline-accent/70duplicates the global rule with a different opacity value.This same pattern appears on the run command button (line 188) and create command button (line 236).
♻️ Suggested fix to remove redundant outline class
- class="px-2 py-0.5 font-mono text-xs text-fg-muted bg-bg-subtle/80 border border-border rounded transition-colors duration-200 opacity-0 group-hover/installcmd:opacity-100 hover:(text-fg border-border-hover) active:scale-95 focus-visible:opacity-100 focus-visible:outline-accent/70" + class="px-2 py-0.5 font-mono text-xs text-fg-muted bg-bg-subtle/80 border border-border rounded transition-colors duration-200 opacity-0 group-hover/installcmd:opacity-100 hover:(text-fg border-border-hover) active:scale-95 focus-visible:opacity-100"Apply similar changes to lines 188 and 236.
Based on learnings: "In the npmx.dev project, focus-visible styling for button and select elements is applied globally via app/assets/main.css... Individual inline utility classes like
focus-visible:outline-accent/70are not needed on these elements."app/components/ReadmeTocDropdown.vue (1)
148-157: Consider relying on global focus-visible styling for buttons.This button has an inline
focus-visible:outline-accent/70utility class. Per the project's established pattern, focus-visible styling for buttons is applied globally viamain.csswithoutline: 2px solid var(--accent). The inline utility may conflict with or duplicate the global rule.If the 70% opacity is intentional for this specific case, consider documenting why it differs from the global standard.
Based on learnings: "In the npmx.dev project, focus-visible styling for button and select elements is applied globally via app/assets/main.css... Do not apply per-element inline utility classes like focus-visible:outline-accent/70 on these elements."
app/pages/search.vue (1)
641-647: Inline focus-visible styling on button may be redundant.This button and the one at Line 739 have inline
focus-visible:outline-accent/70utilities. Per project conventions, buttons should rely on the global focus-visible rule inmain.cssrather than per-element inline classes.Based on learnings: "In the npmx.dev project, focus-visible styling for buttons and selects is applied globally via main.css... individual buttons or selects in Vue components should not rely on inline focus-visible utility classes."
app/components/Org/OperationsQueue.vue (2)
257-263: Inconsistent focus-visible styling on OTP submit button.This button uses
focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-amber-500/50, which differs from all other buttons in this file that usefocus-visible:outline-accent/70. This creates visual inconsistency in focus indicators.Consider aligning with the other buttons, or if the amber colour is intentional for the OTP context, document why this button requires different treatment.
♻️ Suggested fix for consistency
<button type="submit" :disabled="!otpInput.trim() || isExecuting" - class="px-3 py-1.5 font-mono text-xs text-bg bg-amber-500 rounded transition-all duration-200 hover:bg-amber-400 disabled:opacity-50 disabled:cursor-not-allowed focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-amber-500/50" + class="px-3 py-1.5 font-mono text-xs text-bg bg-amber-500 rounded transition-all duration-200 hover:bg-amber-400 disabled:opacity-50 disabled:cursor-not-allowed focus-visible:outline-accent/70" >
277-289: Leftover ring classes on execute button.Line 281 has both
focus-visible:outline-accent/70andfocus-visible:ring-offset-2 focus-visible:ring-offset-bg. The ring-offset classes appear to be remnants from the old ring-based focus styling and have no effect without a corresponding ring utility.♻️ Remove unused ring-offset classes
<button v-if="hasApprovedOperations && !hasOtpFailures" type="button" :disabled="isExecuting" - class="flex-1 px-4 py-2 font-mono text-sm text-bg bg-fg rounded-md transition-all duration-200 hover:bg-fg/90 disabled:opacity-50 disabled:cursor-not-allowed focus-visible:outline-accent/70 focus-visible:ring-offset-2 focus-visible:ring-offset-bg" + class="flex-1 px-4 py-2 font-mono text-sm text-bg bg-fg rounded-md transition-all duration-200 hover:bg-fg/90 disabled:opacity-50 disabled:cursor-not-allowed focus-visible:outline-accent/70" `@click`="handleExecute()" >app/utils/prehydrate.ts (1)
17-35: Hardcoded colours must stay synchronised withconstants.ts.The
colorsobject here is intentionally hardcoded (as Nuxt stringifies theonPrehydratecallback). Ensure this remains synchronised withACCENT_COLORSinshared/utils/constants.tswhen colours are added or modified.Consider adding a comment referencing the source of truth to aid future maintenance.
📝 Add synchronisation comment
// Accent colors - hardcoded since ACCENT_COLORS can't be referenced + // Keep in sync with ACCENT_COLORS in shared/utils/constants.ts const colors = {
| type="button" | ||
| class="px-2 py-0.5 font-mono text-xs text-fg-muted bg-bg-subtle/80 border border-border rounded transition-colors duration-200 opacity-0 group-hover/executecmd:opacity-100 hover:(text-fg border-border-hover) active:scale-95 focus-visible:opacity-100 focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-fg/50" | ||
| class="px-2 py-0.5 font-mono text-xs text-fg-muted bg-bg-subtle/80 border border-border rounded transition-colors duration-200 opacity-0 group-hover/executecmd:opacity-100 hover:(text-fg border-border-hover) active:scale-95 focus-visible:opacity-100 focus-visible:outline-accent/70" | ||
| :aria-label="$t('package.get_started.copy_command')" |
There was a problem hiding this comment.
I think these type of buttons should announce when copied with an aria-live section somewhere.... (probably beyond the scope of this PR, but just mentioning it as I saw your last commit)
There was a problem hiding this comment.
so that'd be something like this?
<div aria-live="polite" aria-atomic="true" class="sr-only">
{{ copiedPkgName ? $t('common.copied') : '' }}
</div>my naive guess with quick ai check.
|
just pushed a few changes to apply what you've done to more components, as well as address some clipping issues in the sidebar would you check to make sure that everything still looks good to you? 🙏❤️ |
all clear 🫡 |
| <span | ||
| class="relative inline-flex h-6 w-11 shrink-0 items-center rounded-full border-2 transition-colors duration-200 ease-in-out motion-reduce:transition-none cursor-pointer" | ||
| :class="checked ? 'bg-accent border-transparent shadow-sm' : 'bg-bg border border-border'" | ||
| :class="checked ? 'bg-accent border-transparent shadow-sm' : 'bg-bg border border-border/50'" |
There was a problem hiding this comment.
Can you explain this change? It seems to have just reduced the contrast for this border and I can't work out the upside from it?
There was a problem hiding this comment.
on light mode borders came out looking really harsh, so for a good visual effect modified the opacity. also, It passes accessibility tests.
There was a problem hiding this comment.
Hmm, on dark mode its pretty borderline to my eyes. Perhaps we can remove the opacity when prefers-contrast: more
There was a problem hiding this comment.
then i suppose it's better without opacity, since there are plans for redesign. what do you think?
There was a problem hiding this comment.
To my eyes it's better without opacity but I use high contrast mode so I mainly care about what it looks like in that mode. I'm happy to defer on standard mode if it's too contrasty for some, then a contrast-more:border-border should fix override the standard one with opacity.
|
This PR seems to have reverted #881 |
4 participants
closes #611
closes #633