Repository navigation
fix: template card detail and text - #7792
Conversation
| <el-button class="flex-1!" type="primary" :disabled="disabled" @click="handleUseTemplate">应用</el-button> | ||
| </div> | ||
| </component> | ||
| </template> |
There was a problem hiding this comment.
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
.stopmodifier.
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
handleApplyTemplateto 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
MkResourceCardexpose a standardclassListor pass through attributes? Most Vue components do viainheritAttrs, but if the root element ofMkResourceCardis 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 ensureMkResourceCardsupportsstyle-classpassthrough. Alternatively, define the cursor behavior internally inMkResourceCardif it's interactive, or use:deepif 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
templateobject is always defined before rendering to avoid[Vue warn]: TypeError: Cannot read property 'name' of undefinedduring initial render or data load delays. Use optional chainingtemplate?.nameiftemplatemight 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:
- Rename
handleUseTemplatetohandleApplyTemplatefor consistency. - Verify that
MkResourceCardsupports theclassand@clickprops. If it doesn't, wrap the card in a native<div>with those properties instead. - 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.
- Add optional chaining (
template?.name) for safety.
| <MkResourceCard :title="tool.name" class="cursor-pointer" @click="openDetailDrawer"> | ||
| <template #icon> | ||
| <ToolIcon :icon="tool.icon" :size="24" :type="tool.tool_type" /> | ||
| </template> |
There was a problem hiding this comment.
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
toolwith a default value referencing an external variable (props.tool) inside a script setup or composition context. If this is a plainfunctiondeclaration inside<script>but outside a setup block,propsmight not be in scope, causing a ReferenceError. If it's insidesetup, usingprops.toolas a default parameter in a standalone function definition is unconventional. - Suggestion: Define the function to explicitly rely on
props.toolor pass arguments correctly from the template/method calls. IfhandleApplyis meant to use the current tool, accessprops.tooldirectly 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 suggestshandleApplyis related to applying/using the tool. EnsureopenDetailDraweris the intended action. IfhandleApplywas previously used, verify if behavior changed unintentionally. - Optimization: If
openDetailDrawerdoesn’t need event details (like$event), this is fine. If you plan to optimize future changes, consider usingv-on="onClickHandlers"or similar patterns if multiple handlers are needed.
3. Accessibility (a11y)
- Issue: A
<template>component (MkResourceCard) likely renders adivorliby default. Adding a@clickmakes it interactive, but non-interactive HTML elements are not accessible to keyboard-only users (which are essential for screen readers). - Suggestion:
- Either ensure
MkResourceCardinternally 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"andaria-labelif semantic HTML isn’t used.
- Either ensure
4. CSS Class Specificity (.cursor-pointer)
- Observation: Hardcoding utility classes like
cursor-pointeris common in Tailwind setups but can lead to inconsistency if other cards don’t have it. Consider defining a custom class.resource-cardthat includescursor: pointer;if this style is universal for all resource cards.
5. Vue Version Compatibility
- If using Vue 2, ensure that
MkResourceCardemits 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>
【【智能体、知识库、工具】1、模板中心、工具商店:点击卡片或者【详情】显示详情;2、按钮文案【使用】改为【应用】】
https://www.tapd.cn/tapd_fe/45245355/bug/detail/1145245355001079067