Skip to content

feat(spx-gui): add UIButtonTest #2879

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.

Sign up for GitHub

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

Open
qingqing-ux wants to merge 1 commit into
base: ui
Choose a base branch
from
+119 −1
Open

feat(spx-gui): add UIButtonTest #2879

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 107 additions & 0 deletions spx-gui/src/components/ui/UIButtonTest.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
<template>
<button
class="ui-button-test"
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The component name UIButtonTest.vue and its class ui-button-test could be misleading. The Test suffix often implies temporary or experimental code not meant for production. If this is a permanent addition to your UI library, consider a more descriptive name that reflects its style (e.g., UIShadowButton) to improve clarity and maintainability.

:class="{ disabled, loading }"
Copy link
Copy Markdown
Contributor

@xgopilot xgopilot bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .disabled CSS class is bound in :class="{ disabled, loading }" but is never targeted by any selector in the stylesheet — all disabled styling uses the native :disabled pseudo-class. The class binding is dead weight.

Additionally, loading does not apply .disabled, so during a loading state the button has no .disabled class despite being non-interactive. This diverges from UIButton.vue's pattern where computed(() => props.disabled || props.loading) keeps both states unified.

:disabled="disabled || loading"
:type="htmlType"
>
<div class="content">
<UIIcon v-if="loading" class="icon" type="loading" />
<UIIcon v-else-if="icon" class="icon" :type="icon" />
<slot v-else name="icon"></slot>
<slot></slot>
</div>
</button>
</template>

<script setup lang="ts">
import { computed } from 'vue'
import UIIcon, { type Type as IconType } from './icons/UIIcon.vue'

const props = withDefaults(
defineProps<{
icon?: IconType
disabled?: boolean
loading?: boolean
htmlType?: 'button' | 'submit' | 'reset'
}>(),
{
icon: undefined,
disabled: false,
loading: false,
htmlType: 'button'
}
)

const disabled = computed(() => props.disabled)
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This computed property is redundant. In Vue 3's <script setup>, props are reactive and can be used directly in the template. You can remove this line to simplify the component's script without changing its behavior.

Copy link
Copy Markdown
Contributor

@xgopilot xgopilot bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This computed is a no-op pass-through — it adds a reactive watcher and dependency hop with no transformation. UIButton.vue's equivalent is meaningful because it merges props.disabled || props.loading. Here it should either be dropped (use props.disabled directly in the template) or align with UIButton's pattern:

Suggested change
const disabled = computed(() => props.disabled)
const disabled = computed(() => props.disabled || props.loading)

If this is aligned with UIButton, the :disabled="disabled || loading" binding on line 5 and the :class binding can also be simplified accordingly.

</script>

<style lang="scss" scoped>
.ui-button-test {
display: flex;
flex-direction: column;
justify-content: flex-end;
align-items: center;
background: none;
border: none;
cursor: pointer;
padding: 0 0 4px 0;
height: 40px; // Total height of 40px (36px content + 4px shadow offset)
Copy link
Copy Markdown
Contributor

@xgopilot xgopilot bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says "36px content + 4px shadow offset = 40px" but the 4px comes from padding-bottom (layout), not the box-shadow (which is visual-only and outside the layout flow). The shadow doesn't contribute to the 40px height at all. Consider:

Suggested change
height: 40px; // Total height of 40px (36px content + 4px shadow offset)
height: 40px; // 36px content + 4px padding-bottom (for the press/shadow illusion)

border-radius: 12px;
outline: none;
transition: all 0.2s cubic-bezier(0.4, 0, 0.2, 1);
Copy link
Copy Markdown
Contributor

@xgopilot xgopilot bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

transition: all watches every animatable CSS property on this element. In practice the only properties that change across states are padding-bottom (on :active) and box-shadow (on .content). Using all means:

  1. The browser evaluates every animatable property on every state change.
  2. padding-bottom is now animated, which triggers a layout reflow on every animation frame for the full 200ms duration of the transition. On a 60fps display that's up to 12 consecutive reflows per button press.

Prefer enumerating only the properties that actually change:

Suggested change
transition: all 0.2s cubic-bezier(0.4, 0, 0.2, 1);
transition: padding-bottom 0.2s cubic-bezier(0.4, 0, 0.2, 1);

(And set transition: box-shadow 0.2s ... explicitly on .content instead of inherit.)


.content {
flex: 1;
width: 100%;
display: flex;
justify-content: center;
align-items: center;
padding: 0 24px;
border-radius: 12px;
font-size: 15px;
line-height: 1.6;
gap: 8px;
font-weight: normal;
color: var(--ui-color-grey-100);
background-color: var(--ui-color-primary-main);
box-shadow: 0 4px var(--ui-color-primary-700);
transition: inherit;
font-family: var(--ui-font-family-main);
}

.icon {
width: 18px;
height: 18px;
}

&:hover:not(:disabled):not(.loading) {
.content {
background-color: var(--ui-color-primary-400);
}
}

&:active:not(:disabled):not(.loading),
&.loading {
padding-bottom: 0;
.content {
box-shadow: none;
}
}

&:disabled:not(.loading) {
cursor: not-allowed;
.content {
// These colors are specifically requested in the design draft (via variables in colors.ts)
color: var(--ui-color-primary-700); // Specified as $turquoise700
Copy link
Copy Markdown
Contributor

@xgopilot xgopilot bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The $turquoise700 / $grey300 / $grey500 notation doesn't exist in this codebase — the project uses CSS custom properties derived from a TypeScript token system, not SCSS $variables. $turquoise700 also bypasses the semantic naming layer; the intended reference is --ui-color-primary-700 (where primary aliases turquoise). The comments are likely meant to trace back to the design token but the notation is misleading.

background-color: var(--ui-color-grey-300); // Specified as $grey300
box-shadow: 0 4px var(--ui-color-grey-500); // Specified as $grey500
}
}

&:focus-visible {
outline: 2px solid var(--ui-color-primary-700);
outline-offset: 2px;
}
}
</style>
1 change: 1 addition & 0 deletions spx-gui/src/components/ui/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ export { default as UIConfigProvider, useUIVariables, type Config } from './UICo
export { default as UICard } from './UICard.vue'
export { default as UICardHeader } from './UICardHeader.vue'
export { default as UIButton } from './UIButton.vue'
export { default as UIButtonTest } from './UIButtonTest.vue'
Copy link
Copy Markdown
Contributor

@xgopilot xgopilot bot Feb 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exporting UIButtonTest from the public UI library index with a Test suffix is a naming concern — it signals a prototype/WIP component rather than a stable one, yet it sits alongside production exports like UIButton, UICard, etc.

More importantly, after comparing both components: UIButtonTest appears to be functionally equivalent to UIButton with variant="shadow", color="primary", and size="large" — all styles (height, padding, colors, shadow offset, hover/active/disabled states) map directly to that configuration. This creates a maintenance fork where future visual changes need to be applied in two places.

Could you clarify the intent? If this is meant to be a simplified preset or a replacement for UIButton, it would be worth either (a) giving it a descriptive name reflecting its purpose, or (b) making it a thin wrapper around UIButton with the relevant props preset.

export { default as UIDropdown, type Pos as DropdownPos } from './UIDropdown'
export { default as UITooltip } from './UITooltip.vue'
export { UIMenu, UIMenuGroup, UIMenuItem } from './menu'
Expand Down
12 changes: 11 additions & 1 deletion spx-gui/src/pages/docs/ui-design.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,16 @@
</div>
</div>
</div>
<div class="component-wrapper">
<h3>UIButtonTest</h3>
<div class="content-wrapper">
<div class="item">
<UIButtonTest icon="play">Default</UIButtonTest>
<UIButtonTest icon="play" disabled>Disabled</UIButtonTest>
<UIButtonTest icon="play" loading>Loading</UIButtonTest>
</div>
</div>
</div>
<div class="component-wrapper">
<h3>UITag</h3>
<div class="setting-wrapper">
Expand Down Expand Up @@ -328,7 +338,7 @@
<script setup lang="ts">
import { ref } from 'vue'

import { UIButton, UISelect, UISelectOption, UITag, useMessage } from '@/components/ui'
import { UIButton, UIButtonTest, UISelect, UISelectOption, UITag, useMessage } from '@/components/ui'
import CommunityNavbar from '@/components/community/CommunityNavbar.vue'
import CenteredWrapper from '@/components/community/CenteredWrapper.vue'
import CommunityFooter from '@/components/community/footer/CommunityFooter.vue'
Expand Down
Loading