-
-
Notifications
You must be signed in to change notification settings - Fork 245
feat: tweak button & link styles #1163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
c76cc87
9b7add1
881330f
558bcad
5bc5e5a
8fedbdd
1a85202
dd19dda
af5b977
49195a6
faabc73
67858b5
2442382
f4e0178
5488d39
f0443a0
1ae70e7
5e64320
dc2f1e0
469ec33
aa3d772
dc748ed
d0dd0ed
3c00392
5ccfa5c
e9b9527
837c94b
9241f2a
135a218
3fa2016
c04c27e
cd69d75
270fd6a
31d10ed
f823696
c31ed68
754f762
f971245
be46382
ee78aef
2b855e3
b804219
419296d
cbde551
ed46b9a
97a4282
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,10 +5,9 @@ const props = withDefaults( | |
| type?: 'button' | 'submit' | ||
| variant?: 'primary' | 'secondary' | ||
| size?: 'small' | 'medium' | ||
| classicon?: string | ||
| ariaKeyshortcuts?: string | ||
| block?: boolean | ||
|
|
||
| classicon?: string | ||
| }>(), | ||
| { | ||
| type: 'button', | ||
|
|
@@ -28,16 +27,15 @@ defineExpose({ | |
| <template> | ||
| <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 gap-x-1.5 relative items-center justify-center rounded-md active:scale-[0.98] font-mono border border-solid border-border transition-colors duration-200 outline-transparent focus-visible:(outline-2 outline-accent outline-offset-2) disabled:(opacity-40 cursor-not-allowed border-transparent)" | ||
| :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, | ||
jellydeck marked this conversation as resolved.
Show resolved
Hide resolved
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Comment on lines
31
to
+38
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Formally we have a chance that button will be disabled without this prop. It would be better to style disabled buttons using
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
do you mean this?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is also a comment about this being a regression that is marked as resolved, but it isn't. |
||
| }" | ||
| :type="props.type" | ||
| :disabled=" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,8 @@ | ||
| <template> | ||
| <div class="relative flex min-w-28 justify-end"> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. min-w-28 was to reduce CLS for authorized users (same in AccountMenu)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i see, i'll use same |
||
| <div | ||
| class="inline-flex gap-x-1 items-center justify-center font-mono border border-border rounded-md text-sm px-4 py-2 bg-transparent text-fg border-none" | ||
| > | ||
| <span class="font-mono text-sm text-fg-muted">{{ $t('account_menu.connect') }}</span> | ||
| <span class="i-carbon-chevron-down w-3 h-3 text-fg-muted" aria-hidden="true" /> | ||
| </div> | ||
| <div | ||
| class="inline-flex gap-x-1.5 min-w-28 items-center justify-center font-mono rounded-md text-sm px-4 py-2 bg-transparent text-fg" | ||
| > | ||
| <span class="text-sm text-fg-muted">{{ $t('account_menu.connect') }}</span> | ||
| <span class="i-carbon-chevron-down w-3 h-3 text-fg-muted" aria-hidden="true" /> | ||
| </div> | ||
| </template> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,26 +70,27 @@ const isButtonMedium = computed(() => props.size === 'medium' && !isLink.value) | |
| 'text-bg bg-fg': variant === 'button-primary', | ||
| 'bg-transparent text-fg': variant === 'button-secondary', | ||
| }" | ||
| ><slot | ||
| /></span> | ||
| > | ||
| <slot /> | ||
| </span> | ||
| <NuxtLink | ||
| v-else | ||
| class="group/link gap-x-1 items-center" | ||
| class="group/link 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" | ||
| :class="{ | ||
| 'flex': block, | ||
| 'inline-flex': !block, | ||
| '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 text-fg/80) 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': | ||
| isButton, | ||
| 'text-sm px-4 py-2': isButtonMedium, | ||
| 'text-xs px-2 py-0.5': isButtonSmall, | ||
| 'bg-transparent text-fg hover:(bg-fg/10 text-accent) focus-visible:(bg-fg/10 text-accent) aria-[current=true]:(bg-fg/10 text-accent border-fg/20 hover:enabled:(bg-fg/20 text-fg/50))': | ||
| variant === 'button-secondary', | ||
| 'text-bg bg-fg hover:(bg-fg/50 text-accent) focus-visible:(bg-fg/50) aria-current:(bg-fg text-bg border-fg hover:enabled:(text-bg/50))': | ||
| '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', | ||
|
Comment on lines
+90
to
+93
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since it's going to be a button and on first class we have |
||
| }" | ||
| :to="to" | ||
| :aria-keyshortcuts="ariaKeyshortcuts" | ||
|
|
@@ -105,7 +106,7 @@ const isButtonMedium = computed(() => props.size === 'medium' && !isLink.value) | |
| /> | ||
| <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" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! |
||
| aria-hidden="true" | ||
| /> | ||
| <kbd | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -88,7 +88,7 @@ function handleKeydown(event: KeyboardEvent) { | |
| <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" | ||
|
Comment on lines
88
to
+91
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are a lot of vanilla
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, i wanted to keep scope of pr limited hence just updated to sync with current buttons style for now |
||
| :aria-expanded="isOpen" | ||
| aria-haspopup="listbox" | ||
| :aria-label="$t('package.get_started.pm_label')" | ||
|
|
@@ -150,7 +150,7 @@ function handleKeydown(event: KeyboardEvent) { | |
| :key="pm.id" | ||
| role="option" | ||
| :aria-selected="selectedPM === pm.id" | ||
| class="cursor-pointer flex items-center gap-2 px-3 py-1.5 font-mono text-xs transition-colors duration-150" | ||
| class="flex items-center gap-2 px-3 py-1.5 font-mono text-xs transition-color cursor-pointer duration-150" | ||
| :class="[ | ||
| selectedPM === pm.id ? 'text-fg' : 'text-fg-subtle', | ||
| highlightedIndex === index ? 'bg-bg-elevated' : 'hover:bg-bg-elevated', | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add outline-offset to prevent clipped focus rings in Firefox.
Line 225 sets
outline: autofor:-moz-focusringwithout 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