Skip to content

fix: template card detail and text #7792

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

Merged
fit2cloudwxx merged 1 commit into from
Nov 10, 2026
Merged

fix: template card detail and text #7792

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
2 changes: 1 addition & 1 deletion ui/src/views/tool/tool-store/component/ToolStoreCard.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ function handleApply(tool = props.tool) {
</script>

<template>
<MkResourceCard :title="tool.name">
<MkResourceCard :title="tool.name" class="cursor-pointer" @click="openDetailDrawer">
<template #icon>
<ToolIcon :icon="tool.icon" :size="24" :type="tool.tool_type" />
</template>

@shaohuzhang1 shaohuzhang1 Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Here are the potential issues and optimization suggestions for the provided code snippet:

1. Prop Validation Issue (function handleApply(tool = props.tool) {)

  • Irregularity: You are defining a function parameter tool with a default value referencing an external variable (props.tool) inside a script setup or composition context. If this is a plain function declaration inside <script> but outside a setup block, props might not be in scope, causing a ReferenceError. If it's inside setup, using props.tool as a default parameter in a standalone function definition is unconventional.
  • Suggestion: Define the function to explicitly rely on props.tool or pass arguments correctly from the template/method calls. If handleApply is meant to use the current tool, access props.tool directly inside the function body instead of using it as a parameter default unless necessary for reusability.

2. Event Handling & Performance (@click="openDetailDrawer" vs @click="handleApply")

  • Potential Issue: The diff changes the click handler from presumably nothing or something else to openDetailDrawer. However, the file name or context suggests handleApply is related to applying/using the tool. Ensure openDetailDrawer is the intended action. If handleApply was previously used, verify if behavior changed unintentionally.
  • Optimization: If openDetailDrawer doesn’t need event details (like $event), this is fine. If you plan to optimize future changes, consider using v-on="onClickHandlers" or similar patterns if multiple handlers are needed.

3. Accessibility (a11y)

  • Issue: A <template> component (MkResourceCard) likely renders a div or li by default. Adding a @click makes it interactive, but non-interactive HTML elements are not accessible to keyboard-only users (which are essential for screen readers).
  • Suggestion:
    • Either ensure MkResourceCard internally handles keyboard events (e.g., tabindex="0" + @keydown.enter).
    • Or add tabindex="0" and @keydown.enter="openDetailDrawer" manually if the component doesn’t support it.
    • Add role="button" and aria-label if semantic HTML isn’t used.

4. CSS Class Specificity (.cursor-pointer)

  • Observation: Hardcoding utility classes like cursor-pointer is common in Tailwind setups but can lead to inconsistency if other cards don’t have it. Consider defining a custom class .resource-card that includes cursor: pointer; if this style is universal for all resource cards.

5. Vue Version Compatibility

  • If using Vue 2, ensure that MkResourceCard emits or supports native DOM events properly. In Vue 3, fall-through attributes work seamlessly. Verify based on your project’s Vue version.

Recommended Improved Code Snippet:

<script setup>
// Example correction for handleApply if it's part of setup logic
// Instead of: function handleApply(tool = props.tool) { ... }
// Prefer explicit access:
const handleApply = () => {
  const toolToApply = props.tool; // Use reactive props directly
  // logic...
};

// Ensure openDetailDrawer is defined
const openDetailDrawer = () => {
  // drawer logic...
};
</script>

<template>
  <MkResourceCard 
    :title="tool.name" 
    class="cursor-pointer" 
    @click="openDetailDrawer"
    tabindex="0" 
    role="button" 
    :aria-label="'Open details for ' + tool.name"
    @keydown.enter.prevent="openDetailDrawer"
    @keydown.space.prevent="openDetailDrawer"
  >
    <template #icon>
      <ToolIcon :icon="tool.icon" :size="24" :type="tool.tool_type" />
    </template>
  </MkResourceCard>
</template>

Expand Down
6 changes: 3 additions & 3 deletions ui/src/views/workflow/components/template-store/components/TemplateCard.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ function handleUseTemplate() {
</script>

<template>
<MkResourceCard :title="template.name">
<MkResourceCard :title="template.name" class="cursor-pointer" @click="handleOpenDetail">
<template #icon>
<el-avatar class="bg-transparent!" shape="square" :size="24">
<img :src="resetUrl(template.icon, true)" alt="" />
Expand All @@ -48,8 +48,8 @@ function handleUseTemplate() {
<div class="flex min-w-0 flex-1" @click.stop>
<!-- 查看模板详情 -->
<el-button class="flex-1!" plain @click="handleOpenDetail">详情</el-button>
<!-- 使用模板 -->
<el-button class="flex-1!" type="primary" :disabled="disabled" @click="handleUseTemplate">使用</el-button>
<!-- 应用模板 -->
<el-button class="flex-1!" type="primary" :disabled="disabled" @click="handleUseTemplate">应用</el-button>
</div>
</component>
</template>

@shaohuzhang1 shaohuzhang1 Nov 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Here is the review of the provided Vue code snippet, highlighting irregularities, potential issues, and optimization suggestions.

1. Conflicting Click Handlers (Critical Issue)

Issue: You added a @click event to the outer <MkResourceCard> that triggers handleOpenDetail. However, inside the component, there is already a button (详情 / Details) that also calls handleOpenDetail.

  • Risk: If the user clicks anywhere on the card other than the inner buttons, it will trigger handleOpenDetail. This might be intended ("click card to view details"), but it creates two ways to trigger the same action.
  • Conflict: If the intention was that clicking the card opens the detail modal, ensure that the inner "Details" button is redundant or behaves differently. If the inner button must remain, consider making the outer click handler conditional so it doesn't double-fire if the user accidentally clicks near the button but outside its specific stop-propagation area.

Suggestion:
If the whole card should open details, remove the outer @click and rely on the inner button, or conversely, remove the inner "Details" button if the card click is sufficient. If both must exist, ensure UX clarity.

2. @click.stop Placement and Efficiency

Observation: The inner <div class="flex min-w-0 flex-1" @click.stop> prevents propagation from the two buttons to the parent card.

  • Current State: This correctly prevents the card's new @click="handleOpenDetail" from firing when either "Details" or "Apply" buttons are clicked.
  • Assessment: This logic is correct given the change in line 33. No change needed here, but it highlights the dependency: the parent click handler relies on this .stop modifier.

3. Naming Convention Inconsistency

Change: You changed "使用模板" (Use Template) to "应用模板" (Apply Template) in the comment, and "使用" to "应用" on the button.
Function Name: The function is still named handleUseTemplate.

  • Issue: The UI text says "Apply" ("应用"), but the code variable/function name says "Use" (handleUseTemplate).
  • Suggestion: For maintainability, rename the function to handleApplyTemplate to keep the code semantics aligned with the UI terminology. This prevents confusion for future developers.

4. CSS Class cursor-pointer

Observation: Added class="cursor-pointer" to <MkResourceCard>.
Issue:

  • Does MkResourceCard expose a standard classList or pass through attributes? Most Vue components do via inheritAttrs, but if the root element of MkResourceCard is not a native DOM element (e.g., another wrapper), styles might not apply as expected.
  • Best Practice: Instead of adding a utility class to a potentially custom component, it’s safer to add the style to a wrapping <div> or ensure MkResourceCard supports style-class passthrough. Alternatively, define the cursor behavior internally in MkResourceCard if it's interactive, or use :deep if styling globally.

5. Accessibility (a11y) Suggestion

Issue: You made the entire card clickable (<div> effectively, or component root) but did not specify an ARIA role.
Suggestion: To improve accessibility, ensure the card has role="button" or wrap the interactive part in an actual <button> / <a> tag, or add aria-labels. Screen readers might not recognize the card as interactive unless handled properly by the underlying component structure.

6. Minor Syntax/Style Check

<MkResourceCard :title="template.name" class="cursor-pointer" @click="handleOpenDetail">
  • Check: Ensure template object is always defined before rendering to avoid [Vue warn]: TypeError: Cannot read property 'name' of undefined during initial render or data load delays. Use optional chaining template?.name if template might be null.

✅ Recommended Refactored Code

<script setup lang="ts">
// Rename function to match UI label "Apply"
function handleUseTemplate() {
  // ... implementation
}
</script>

<template>
  <!-- 
    Option A: If the whole card is clickable for details:
    Ensure MkResourceCard passes through attrs/class. 
    Consider removing the inner "Details" button to simplify UX if redundant.
  -->
  <MkResourceCard 
    :title="template?.name" 
    class="cursor-pointer group-hover:bg-color-bg-subtle transition-colors" 
    @click="handleOpenDetail"
  >
    <template #icon>
      <el-avatar class="bg-transparent!" shape="square" :size="24">
        <img :src="resetUrl(template.icon, true)" alt="" />
      </el-avatar>
    </template>

    <!-- Content ... -->

    <template #footer>
      <component :is="footerComponent">
        <div class="flex min-w-0 flex-1" @click.stop>
          <!-- Keep only if you want explicit actions separate from general card click -->
          <!-- If card click already does "Details", consider removing this button -->
          <el-button 
            class="flex-1 mr-2!" 
            plain 
            @click.stop="handleOpenDetail" 
          >
            Details
          </el-button>
          
          <el-button 
            class="flex-1!" 
            type="primary" 
            :disabled="disabled" 
            @click.stop="handleUseTemplate" 
          >
            Apply
          </el-button>
        </div>
      </component>
    </template>
  </MkResourceCard>
</template>

Summary of Actions:

  1. Rename handleUseTemplate to handleApplyTemplate for consistency.
  2. Verify that MkResourceCard supports the class and @click props. If it doesn't, wrap the card in a native <div> with those properties instead.
  3. Clarify UX: Decide if "Details" button is needed if the whole card is already clickable for details. If yes, keep it; if no, remove it to reduce clutter.
  4. Add optional chaining (template?.name) for safety.

Expand Down
Loading