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! |
|
jellydeck
left a comment
jellydeck
left a comment
There was a problem hiding this comment.
reverted styles as per discussion
| :class="{ | ||
| 'inline-flex': !block, | ||
| 'flex': block, | ||
| 'text-sm px-4 py-2': size === 'medium', | ||
| 'text-xs px-2 py-0.5': size === 'small', | ||
| 'bg-transparent text-fg hover:enabled:(bg-fg/10) focus-visible:enabled:(bg-fg/10) aria-pressed:(bg-fg/10 border-fg/20 hover:enabled:(bg-fg/20 text-fg/50))': | ||
| variant === 'secondary', | ||
| 'text-bg bg-fg hover:enabled:(bg-fg/50) focus-visible:enabled:(bg-fg/50) aria-pressed:(bg-fg text-bg border-fg hover:enabled:(text-bg/50))': | ||
| variant === 'primary', | ||
| 'text-bg bg-fg border-fg hover:(bg-fg/80)': variant === 'primary', | ||
| 'text-fg bg-bg hover:(bg-fg/10 border-fg/10)': variant === 'secondary', | ||
| 'opacity-40 cursor-not-allowed border-transparent': disabled, |
There was a problem hiding this comment.
disabled:(opacity-40 cursor-not-allowed border-transparent)
https://github.com/npmx-dev/npmx.dev/pull/1163/changes/BASE..be463823d307f24f07a17f37866da9912c961c00#diff-4274e1791d85b6346c5a195d790469b3792aa4a49ddd2874f043048e118ae598R30
do you mean this?
app/components/Button/Group.vue
Outdated
| <component | ||
| :is="props.as || 'div'" | ||
| class="flex items-center shrink-0 ms-auto [&>*:not(:first-child)]:rounded-s-none [&>*:not(:first-child)]:border-s-0 [&>*:not(:last-child)]:rounded-e-none" | ||
| class="inline-flex items-stretch [&>*:not(:first-child)]:(-ms-px rounded-is-none) [&>*:not(:last-child)]:rounded-ie-none [&>*:hover]:(relative z-40) [&>*:focus-visible]:(relative z-40)" |
There was a problem hiding this comment.
disabling scale effect on active and hover
| @@ -1,10 +1,8 @@ | |||
| <template> | |||
| <div class="relative flex min-w-28 justify-end"> | |||
There was a problem hiding this comment.
i see, i'll use same min-w-28 then
app/components/Link/Base.vue
Outdated
| <NuxtLink | ||
| v-else | ||
| class="group/link gap-x-1 items-center" | ||
| class="group cursor-pointer gap-x-1.5 items-center rounded-sm outline-transparent active:scale-[0.98] focus-visible:(outline-2 outline-accent) transition-colors duration-200" |
There was a problem hiding this comment.
alrighty, now all link styles are under group/link variant
app/components/Link/Base.vue
Outdated
| 'justify-start font-mono text-fg hover:(decoration-accent) focus-visible:(text-accent outline-offset-2)': | ||
| isLink, | ||
| 'justify-center font-mono border border-border rounded-md transition-all duration-200': | ||
| 'justify-center font-mono border border-solid border-border rounded-md outline-offset-2': |
There was a problem hiding this comment.
These outline-offsets-2 should be set on the first class because they should apply to all links regardless of type. Also the use is inconsistant, sometimes it's always set, sometimes only on focus-visible.
There was a problem hiding this comment.
for button i used offset-2 otherwise it already has focus-visible:(outline-2 outline-accent) . this is to create a better seperation.
| 'text-bg bg-fg border-fg hover:(bg-fg/80) aria-[current=true]:(bg-fg/80)': | ||
| variant === 'button-primary', | ||
| 'text-fg bg-bg hover:(bg-fg/10 border-fg/10) aria-[current=true]:(bg-fg/10 border-fg/10)': | ||
| variant === 'button-secondary', |
There was a problem hiding this comment.
This doesn't change the style anymore when the button is focused. Is that on purpose or by accident? The same goes for not having the text in accent colors, for button-styled links.
There was a problem hiding this comment.
since it's going to be a button and on first class we have focus-visible:(outline-2 outline-accent) and outline-offset-2 on button.
it will have focus style applied as intended.
app/components/Link/Base.vue
Outdated
| 'underline-offset-[0.2rem] underline decoration-1 decoration-fg/30': | ||
| !isLinkAnchor && isLink && !noUnderline, | ||
| 'justify-start font-mono text-fg hover:(decoration-accent text-accent) focus-visible:(decoration-accent text-accent) transition-colors duration-200': | ||
| 'justify-start font-mono text-fg hover:(decoration-accent) focus-visible:(text-accent outline-offset-2)': |
There was a problem hiding this comment.
This changes doesn't change the text color to accent on hover, but on focus. This seems inconsistant with the button styles.
There was a problem hiding this comment.
text-accent was just too strong, will use shade of fg color if we still need a dim variant
There was a problem hiding this comment.
I think especially on light mode it can be too subtle now.
| <span | ||
| v-else-if="isLinkAnchor && isLink" | ||
| class="i-carbon:link size-[1em] opacity-0 group-hover/link:opacity-100 transition-opacity duration-200" | ||
| class="i-carbon:link size-[1em] opacity-0 group-hover/link:opacity-100 group-focus-visible/link:opacity-100 transition-opacity duration-200" |
| <button | ||
| ref="triggerRef" | ||
| type="button" | ||
| class="cursor-pointer flex items-center gap-1.5 px-2 py-2 font-mono text-xs text-fg-muted bg-bg-subtle border border-border-subtle border-solid rounded-md transition-colors duration-150 hover:(text-fg border-border-hover) active:scale-95 focus:border-border-hover focus-visible:outline-accent/70" | ||
| class="flex items-center gap-1.5 px-2 py-2 font-mono text-xs text-fg-muted bg-bg-subtle border border-border-subtle border-solid rounded-md transition-colors duration-150 hover:(text-fg border-border-hover) active:scale-95 focus:border-border-hover outline-transparent focus-visible:(outline-2 outline-accent outline-offset-2) hover:text-fg cursor-pointer" |
There was a problem hiding this comment.
There are a lot of vanilla button in this PR which are updated. But most of them should be migrated to ButtonBase, instead of manually syncing the style. It is fine like that, but that change will happen soon anyway.
There was a problem hiding this comment.
yeah, i wanted to keep scope of pr limited hence just updated to sync with current buttons style for now
Those two have different variants, which is barely visible now. |
|
@essenmitsosse 
|
5 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 :)