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 = {
| 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