Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
@jellydeck is attempting to deploy a commit to the serhalp's projects Team on Vercel. A member of the Team first needs to authorize it. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
app/components/Button/Base.vue
Outdated
| <span | ||
| v-if="classicon" | ||
| :class="[size === 'small' ? 'size-3' : 'size-4', classicon]" | ||
| aria-hidden="true" | ||
| /> | ||
| <slot /> | ||
| <kbd | ||
| v-if="keyshortcut" | ||
| class="ms-2 inline-flex items-center justify-center w-4 h-4 text-xs text-fg bg-bg-muted border border-border rounded no-underline" | ||
| aria-hidden="true" | ||
| class="group cursor-pointer inline-flex gap-x-1.5 relative items-center justify-center rounded-md active:scale-[0.98] font-mono border border-solid border-border transition-[background-color,color,border,outline] duration-200 transition-[border-radius_100ms] after:(content-[''] absolute inset--0.5 rounded-md) outline-transparent group-focus-visible:(outline-2 outline-accent outline-offset-2)" | ||
| :class="{ | ||
| 'text-sm px-4 py-2': size === 'medium', | ||
| 'text-xs px-2 py-0.5': size === 'small', | ||
| 'text-fg bg-bg hover:(bg-fg/10 border-fg/10)': variant === 'secondary', | ||
| 'text-bg bg-fg border-fg hover:(bg-fg/80 rounded-xl) active:rounded-xl': | ||
| variant === 'primary', | ||
| 'opacity-40 cursor-not-allowed border-transparent': disabled, | ||
| }" | ||
| > |
There was a problem hiding this comment.
What's the reason for the additional wrapper?
There was a problem hiding this comment.
for searchbox on home page, we pass absolute class directly to button which messed with internal classes. to remove that clash i took wrapper approach. so all classes passed directly to button would only handle layout cases and inside with wrapper we manage text & icons
There was a problem hiding this comment.
Would it work to wrap the button in the searchbox instead of adding a wrapper to every button? I wouldn't want to leak the problem of one location into the implementation of an abstract button.
There was a problem hiding this comment.
pushed commits, lemme know what you think
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request standardises focus-visible/focus-outline styling across many UI components and replaces a global custom focus outline in CSS with browser-default behaviour in one rule. It adds a new public prop Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 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 (6)
app/components/Terminal/Install.vue (1)
124-130:⚠️ Potential issue | 🟡 MinorRemove per-button focus-visible outline utilities.
Buttons already receive focus-visible outline styling globally; keeping inline outline utilities here risks inconsistent accent/offsets and clipped rings. Retain only the focus-visible opacity change for visibility.
Proposed fix
- 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-98 focus-visible:(opacity-100 outline-2 outline-accent) select-none" + 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-98 focus-visible:opacity-100 select-none"Based on learnings: 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 likefocus-visible:outline-accent/70on these elements in Vue templates or components.app/components/Package/Dependencies.vue (1)
143-158:⚠️ Potential issue | 🟡 MinorInconsistent focus-visible styling across "Show all" buttons.
The three "Show all" buttons have different focus styling:
- Line 146 (dependencies):
focus-visible:(outline-2 outline-offset-2 outline-accent)✓- Line 198 (peer dependencies):
focus-visible:outline-accent/70(old style)- Line 255 (optional dependencies): No focus-visible styling at all
Consider applying consistent styling across all three buttons.
♻️ Suggested fix for consistency
<button v-if="sortedPeerDependencies.length > 10 && !peerDepsExpanded" type="button" - class="mt-2 font-mono text-xs text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70" + class="my-2 ms-1 font-mono text-xs text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:(outline-2 outline-offset-2 outline-accent)" `@click`="peerDepsExpanded = true" ><button v-if="sortedOptionalDependencies.length > 10 && !optionalDepsExpanded" type="button" - class="mt-2 truncate" + class="my-2 ms-1 font-mono text-xs text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:(outline-2 outline-offset-2 outline-accent)" `@click`="optionalDepsExpanded = true" >Also applies to: 195-210, 252-267
app/components/AppHeader.vue (2)
193-202:⚠️ Potential issue | 🔴 CriticalSyntax error in focus-visible utility class.
Missing colon in the UnoCSS variant grouping syntax.
focus-visible(...)should befocus-visible:(...).🐛 Proposed fix
<button v-if="!isSearchExpanded && !isOnHomePage" type="button" - class="sm:hidden flex-shrink-0 inline-flex items-center gap-2 font-mono text-lg font-medium text-fg hover:text-fg transition-colors duration-200 rounded focus-visible(outline-2 outline-accent)" + class="sm:hidden flex-shrink-0 inline-flex items-center gap-2 font-mono text-lg font-medium text-fg hover:text-fg transition-colors duration-200 rounded focus-visible:(outline-2 outline-accent)" :aria-label="$t('nav.tap_to_search')" `@click`="expandMobileSearch" >
254-275:⚠️ Potential issue | 🟠 MajorDuplicate Compare button rendering.
The Compare link is rendered twice:
- Explicitly at lines 256-263
- Again in the
v-forloop at lines 266-275 (sincedesktopLinksincludes Compare at line 17-26)Either remove Compare from
desktopLinksor filter it out from the loop.🐛 Proposed fix - filter Compare from loop
<!-- Desktop: Settings link --> <LinkBase - v-for="link in desktopLinks" + v-for="link in desktopLinks.filter(l => l.name !== 'Compare')" :key="link.name" class="border-none" variant="button-secondary" :to="link.to" :aria-keyshortcuts="link.keyshortcut" > {{ link.label }} </LinkBase>app/components/Link/Base.vue (1)
36-37:⚠️ Potential issue | 🟡 MinorRemove the unused
noUnderlineprop or restore its functionality.The
noUnderlineprop declared at line 37 is not used anywhere in the component. The underline styling at line 82 applies unconditionally to all links (when!isLinkAnchor && isLink) without checking thenoUnderlineprop. Either remove the prop if it is no longer needed, or add&& !noUnderlineto the conditional at line 82 to restore its intended behaviour.app/pages/settings.vue (1)
52-60:⚠️ Potential issue | 🟡 MinorRemove focus-visible utilities from disabled select element at line 243.
The disabled select cannot receive focus, so the
focus-visible:(outline-2 outline-accent outline-offset-2)class is unnecessary and should be removed from this element.
🧹 Nitpick comments (7)
app/components/LicenseDisplay.vue (1)
21-21: Consider addingoutline-offsetfor consistency.Other interactive elements in this PR use
outline-offset-2to provide spacing between the element and its focus outline. This anchor lacks an offset, which may cause the focus ring to sit directly against the text.💡 Suggested change
- class="focus-visible:(outline-2 outline-accent) rounded-sm" + class="focus-visible:(outline-2 outline-accent outline-offset-2) rounded-sm"app/components/Readme.vue (1)
155-160: Keep the scoped focus-visible rule for anchors only.Line 155 also targets buttons inside
.readme, which duplicates the project’s global button focus styling and can introduce conflicts (especially with.readmeusing overflow clipping). Consider scoping this rule to anchors only.Suggested fix
-.readme :deep(a:focus-visible), -.readme :deep(button:focus-visible) { +.readme :deep(a:focus-visible) { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }Based on learnings: In the npmx.dev project, focus-visible styling for buttons and select elements is applied globally via main.css using the rule:
button:focus-visible, select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }. Therefore, individual buttons and selects don't need inline focus-visible utility classes.app/components/Tag/Static.vue (1)
16-17: Apply focus-visible utilities only for non-button rendering.The static focus-visible class applies even when
asisbutton/select, where the global rule already handles focus. Consider applying it only for non-button/select render targets.Suggested fix
<component :is="as" - class="bg-bg-muted text-fg-muted inline-flex gap-x-1 items-center justify-center font-mono border border-transparent rounded-md text-xs px-2 py-0.5 focus-visible:(outline-2 outline-accent)" - :class="{ 'opacity-40': variant === 'ghost' }" + class="bg-bg-muted text-fg-muted inline-flex gap-x-1 items-center justify-center font-mono border border-transparent rounded-md text-xs px-2 py-0.5" + :class="[ + { 'opacity-40': variant === 'ghost' }, + typeof props.as === 'string' && !['button', 'select'].includes(props.as) + ? 'focus-visible:(outline-2 outline-accent)' + : null, + ]" >Based on learnings: In the npmx.dev project, focus-visible styling for buttons and select elements is applied globally via main.css using the rule:
button:focus-visible, select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }. Therefore, individual buttons and selects don't need inline focus-visible utility classes.app/components/Compare/FacetSelector.vue (1)
72-72: Remove redundant focus-visible utilities onButtonBase.Line 72 adds focus-visible outline utilities even though
ButtonBasealready provides them, increasing class noise and potential divergence. Consider removing the duplicate focus-visible classes.Suggested fix
- class="inline-flex items-center gap-1 px-1.5 py-0.5 font-mono text-xs rounded border transition-colors duration-200 focus-visible:(outline-2 outline-accent outline-offset-2)" + class="inline-flex items-center gap-1 px-1.5 py-0.5 font-mono text-xs rounded border transition-colors duration-200"Based on learnings: In the npmx.dev project, focus-visible styling for buttons and select elements is applied globally via main.css using the rule:
button:focus-visible, select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }. Therefore, individual buttons and selects don't need inline focus-visible utility classes.app/components/Button/Base.vue (1)
30-38: Prefer global focus-visible styling over per-button utilities.Line 30 adds explicit focus-visible outline utilities; the project convention is to keep button focus-visible styling global in main.css. Consider removing the per-button focus-visible utilities to avoid divergence.
Suggested fix
- class="group cursor-pointer inline-flex gap-x-1.5 relative items-center justify-center rounded-md active:scale-[0.98] font-mono border border-solid border-border transition-[background-color,color,border,outline] duration-200 outline-transparent focus-visible:(outline-2 outline-accent outline-offset-2)" + class="group cursor-pointer inline-flex gap-x-1.5 relative items-center justify-center rounded-md active:scale-[0.98] font-mono border border-solid border-border transition-[background-color,color,border,outline] duration-200 outline-transparent"Based on learnings: In the npmx.dev project, focus-visible styling for buttons and select elements is applied globally via main.css using the rule:
button:focus-visible, select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }. Therefore, individual buttons and selects don't need inline focus-visible utility classes.app/pages/settings.vue (2)
80-96: Same concern: inline focus-visible utilities on select elements.This select has inline
focus-visible:(outline-2 outline-accent outline-offset-2)utilities, but the search provider select at line 169 does not. If inline utilities are needed, apply them consistently; otherwise, rely on the global CSS rule for all selects.
239-247: Unnecessary focus-visible styling on disabled select.The
disabledattribute prevents this element from receiving focus, so thefocus-visible:(...)utilities will never apply. Consider removing them for clarity.Suggested change
<select id="language-select" disabled - class="w-full sm:w-auto min-w-48 bg-bg border border-border rounded-md px-3 py-2 text-sm text-fg cursor-pointer duration-200 transition-colors hover:border-fg-subtle focus-visible:(outline-2 outline-accent outline-offset-2)" + class="w-full sm:w-auto min-w-48 bg-bg border border-border rounded-md px-3 py-2 text-sm text-fg opacity-50 cursor-wait duration-200 transition-colors" >Note: Also consider using
cursor-waitandopacity-50here to match the disabled search provider fallback at line 186 for visual consistency.
| :-moz-focusring { | ||
| /* weird Firefox behavior makes it necessary to add `!important` | ||
| or otherwise the selector would need to be more specific, | ||
| which it explicitly should not be. */ | ||
| outline: 2px solid var(--accent) !important; | ||
| outline-offset: 2px !important; | ||
| outline: auto; | ||
| } |
There was a problem hiding this comment.
Add outline-offset to prevent clipped focus rings in Firefox.
Line 225 sets outline: auto for :-moz-focusring without an offset; in tight or overflow-clipped layouts this can hide the ring (a reported issue in this PR). Adding an offset keeps the focus indicator visible.
Suggested fix
:-moz-focusring {
outline: auto;
+ outline-offset: 2px;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| :-moz-focusring { | |
| /* weird Firefox behavior makes it necessary to add `!important` | |
| or otherwise the selector would need to be more specific, | |
| which it explicitly should not be. */ | |
| outline: 2px solid var(--accent) !important; | |
| outline-offset: 2px !important; | |
| outline: auto; | |
| } | |
| :-moz-focusring { | |
| outline: auto; | |
| outline-offset: 2px; | |
| } |
There was a problem hiding this comment.
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/Dependencies.vue (1)
143-158:⚠️ Potential issue | 🟡 MinorInconsistent focus-visible styling across "Show all" buttons.
This file contains three "Show all" buttons with different focus styling:
- Line 146: Uses the new explicit styling
focus-visible:(outline-2 outline-offset-2 outline-accent)✓- Line 198: Still uses the old
focus-visible:outline-accent/70style- Line 255: Has no focus-visible styling at all
Since the PR removes global focus CSS and standardises inline focus styling, all three buttons should use the same approach. This may relate to the accent colour inconsistencies reported in the PR review comments.
🔧 Proposed fix to standardise focus styling
Apply the same focus-visible classes to the peer dependencies button:
<button v-if="sortedPeerDependencies.length > 10 && !peerDepsExpanded" type="button" - class="mt-2 font-mono text-xs text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70" + class="my-2 ms-1 font-mono text-xs text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:(outline-2 outline-offset-2 outline-accent)" `@click`="peerDepsExpanded = true" >Apply the same styling to the optional dependencies button:
<button v-if="sortedOptionalDependencies.length > 10 && !optionalDepsExpanded" type="button" - class="mt-2 truncate" + class="my-2 ms-1 font-mono text-xs text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:(outline-2 outline-offset-2 outline-accent)" `@click`="optionalDepsExpanded = true" >Also applies to: 195-210, 252-267
app/components/Button/Base.vue
Outdated
| <button | ||
| ref="el" | ||
| class="group cursor-pointer gap-x-1 items-center justify-center font-mono border border-border rounded-md transition-all duration-200 disabled:(opacity-40 cursor-not-allowed border-transparent)" | ||
| class="group cursor-pointer inline-flex gap-x-1.5 relative items-center justify-center rounded-md active:scale-[0.98] font-mono border border-solid border-border transition-[background-color,color,border,outline] duration-200 outline-transparent focus-visible:(outline-2 outline-accent outline-offset-2)" |
There was a problem hiding this comment.
inline-flex needs to go, this is no handled further down.
There was a problem hiding this comment.
What does active:scale-[0.98] do?
There was a problem hiding this comment.
border-solid should be redundant?
There was a problem hiding this comment.
Is transition-[background-color,color,border,outline] necessary? This will add a lot of classes, is there anything we don't want to animate?
There was a problem hiding this comment.
active scale class if for press effect when we click on button, without border-solid class border weren't showing up.
border class just sets below
unlike tailwind i think we have to specify border-solid here
i was suggested on discord to only animate properties that we use
transition-[background-color,color,border,outline]
There was a problem hiding this comment.
But there was a border before? 🤔
There was a problem hiding this comment.
I would leave the transition- change to a separate PR, if it does not change the design, to not bloat this one to much.
There was a problem hiding this comment.
active scale class if for press effect when we click on button,
Ahhh! got it!
app/components/Link/Base.vue
Outdated
| <NuxtLink | ||
| v-else | ||
| class="group/link gap-x-1 items-center" | ||
| class="group inline-flex gap-x-1 items-center justify-center rounded-sm outline-transparent active:scale-[0.98] focus-visible:(outline-2 outline-accent)" |
There was a problem hiding this comment.
justify-center wouldn't work, since this is a flex or inline-flex
There was a problem hiding this comment.
Why is the focus definition slightly different then the one for button?
There is a regression here, where the menu links/buttons have their outline back. This should be overwritten (see prod) |
| <component | ||
| :is="as" | ||
| class="bg-bg-muted text-fg-muted inline-flex gap-x-1 items-center justify-center font-mono border border-transparent rounded-md text-xs px-2 py-0.5" | ||
| class="bg-bg-muted text-fg-muted inline-flex gap-x-1 items-center justify-center font-mono border border-transparent rounded-md text-xs px-2 py-0.5 focus-visible:(outline-2 outline-accent)" |
There was a problem hiding this comment.
This is a static element and should be able to have focus. If someone uses it as a button or link that would be wrong and it's a case that should be prevented.
There was a problem hiding this comment.
so no focus-visible i suppose, but without that it shows default outlines of browser
There was a problem hiding this comment.
Is there a case where this element can be focused?
| !isLinkAnchor && isLink && !noUnderline, | ||
| 'justify-start font-mono text-fg hover:(decoration-accent text-accent) focus-visible:(decoration-accent text-accent) transition-colors duration-200': | ||
| 'underline-offset-[0.2rem] underline decoration-1 decoration-fg/30': !isLinkAnchor && isLink, | ||
| 'font-mono text-fg hover:(decoration-accent) focus-visible:(decoration-accent outline-offset-2 text-accent) transition-colors duration-200': |
There was a problem hiding this comment.
Is there a reason hover:text-accent was removed? Now the hover effect is much more subtle.
There was a problem hiding this comment.
underline text should use their underline as effect hence hover:text-accent
This looks very tight now |
not intended, will do fix once i'll be back on desktop |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/components/Button/Base.vue (1)
8-8: Consider renamingclassicontoiconClassfor consistency.The prop name
classicondeviates from standard camelCase conventions (e.g.,iconClass). This would align better with typical Vue/JS naming patterns and improve readability.♻️ Suggested rename
- classicon?: string + iconClass?: stringAnd update the template reference at line 52:
- <span v-if="classicon" class="size-[1em]" :class="classicon" aria-hidden="true" /> + <span v-if="iconClass" class="size-[1em]" :class="iconClass" aria-hidden="true" />
4 participants
update the general style of buttons, group buttons and links to be consistent with our design language.
also, removed global css as Input component seems to be done.
Thanks @alexdln @essenmitsosse for feedback, enjoyed working together :)