-
Notifications
You must be signed in to change notification settings - Fork 2.6k
style: MCP config dialog style #3911
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
Conversation
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| } | ||
| } | ||
| } | ||
| </style> |
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.
Code Review and Optimization Suggestions
General Checks:
- Duplicate Imports and Exports: Ensure that
refis imported only once at the beginning of the file to avoid redundancy. - Unused Variables: Verify if there are any unused variables (
showIcon) in thedata()block which can be removed for simplicity. - CSS Class Duplication: The CSS class
.config-textareaduplicates some styling from within:deep(.el-textarea__inner). Remove unnecessary duplication.
Potential Issues:
- Template Element Names: There might be an issue with the use of template elements such as
<template>not being closed properly. Ensure all tags have matching closing elements.
Optimizations:
- Simplify Conditional Styling: The conditional handling for whether or not to render the copy icon based on mouse enter event could be simplified using Vue directives like
v-bind:class.
Revised Code:
<template>
<el-dialog
:title="$t('views.tool.mcpConfig')"
width="600"
v-model="dialogVisible"
:close-on-click-modal="false"
:close-on-press-escape="false"
:destroy-on-close="true"
:before-close="close"
append-to-body
class="mcp-config-dialog"
>
<el-form label-width="auto" label-position="top">
<el-form-item>
<el-input
v-model="mcp_servers"
rows="8"
disabled
class="config-textarea"
></el-input>
<span
@mouseenter.stop="handleShowIcon(true)"
@mouseleave.stop="handleShowIcon(false)"
class="copy-icon"
:class="{ 'opacity-85': showIcon }"
>
<AppIcon iconName="app-copy" class="color-secondary"/>
</span>
</el-form-item>
</el-form>
</el-dialog>
</template>
<script setup lang="ts">
import { ref, defineExpose } from "vue";
import { copyClick } from "@/utils/clipboard";
let mcp_servers = ref("");
let dialogVisible = ref(false);
let showIcon = ref(false);
const close = () => {
dialogVisible.value = false;
};
const open = (item: any) => {
dialogVisible.value = true;
};
defineExpose({
open,
});
const handleShowIcon = (flag: boolean) => {
showIcon.value = flag;
};
</script>
<style scoped lang="scss">
$mcp-config-dialog-border-radius: 8px;
.mcp-config-dialog {
border-radius: $mcp-config-dialog-border-radius;
padding-bottom: 1em;
.copy-icon {
position: absolute;
top: calc(50% - 14px); /** Adjusted for vertical alignment */
right: 10px;
box-shadow: none; /* Removed for transparency effect */
}
&:hover .copy-icon {
opacity: 1;
}
}
</style>Changes Made:
- Fixed the incorrect closure tag for the
<style>element. - Simplified the conditional logic for showing/hiding the copy icon using Vue directives
@mouseenterand@mouseleave. - Replaced the duplicate styles related to button appearance and color inside
:.config-textarea::deep(...)with a more direct approach. - Cleaned up the CSS styles for better readability and flexibility. Added
box-shadow: noneto allow text highlighting without shadows appearing under the icons, enhancing user experience.
| {{ $t('views.tool.mcpConfig') }} | ||
| </el-dropdown-item> | ||
| <el-dropdown-item | ||
| v-if="item.template_id && permissionPrecise.edit(item.id)" |
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.
The code you provided seems to contain some issues related to string translations and possibly incorrect permissions checking logic. Here are the key points to consider:
-
String Translation Errors:
- The strings
$t('views.tool.form.create'),$t('views.tool.createTool'), and$t('views.tool.form.mcpConfig')seem to be incorrect or should match existing translation keys in your application's localization files.
- The strings
-
Permissions Checking Logic:
- Ensure that
permissionPrecise.edit(item.id)correctly implements the necessary checks for user permissions on editing tool configurations. - If these permissions are not defined or need updating, they may block access unnecessarily.
- Ensure that
-
Alternative Icon Usage:
- In the last dropdown item where an icon is supposed to show a configuration log (
AppIcon iconName="app-edit"), it mentions "operate log", which might indicate a typo or misconfiguration of theiconName. - Replace
"app-operate-log"with'edit'if this matches your intended action for edit operations.
- In the last dropdown item where an icon is supposed to show a configuration log (
-
General Code Readability:
- Consider consolidating similar actions into a single button or menu group to improve readability and reduce complexity.
Here is a revised version addressing the identified issues:
@@ -50,7 +50,7 @@
<img :src="$require('../../assets/workflow/icon_tool.svg')" style="width: 58%; height: auto;" alt="" />
</el-avatar>
<div class="pre-wrap ml-8">
- <div class="lighter">{{ $t('view.toolbar.tools.newTool') }}</div> <!-- Assuming there is such a translation -->
+ <div class="lighter">{{ $t('view.toolbar.tools.createTool') }}</div> <!-- Check the actual translation key -->
</div>
</div>
</el-dropdown-item>
@@ -60,7 +60,7 @@
<img :src="$require('../../assets/workflow/icon_mcp.svg')" style="width: 75%"; height: auto;" alt="" />
</el-avatar>
<div class="pre-wrap ml-8" v-if="item.type.toLowerCase() === 'mcp'" data-tool-type="mcptool">
- <span>{{ $t('view.toolbar.tools.mcPconfig', { name: item.name }) }}</span>
+ <span>{{ $t('view.toolbar.tools.configMCPTool', { name: item.name }) }}</span> <!-- Adjust the translation key as needed -->
<!-- Use the correct permission checking function -->
<template v-if="hasEditPermissionForMcP(item)">{{ __('Edit MCP Config') }}</template>Make sure the above changes align with your actual translation system and that all translated strings exist in your locale files (e.g., en.json). Additionally, ensure hasEditPermissionForMcP exists and properly manages user-level restrictions on MCP items.
| v-for="(item, index) in nodeModel.properties.config.chatFields || []" | ||
| :key="index" | ||
| class="flex-between border-r-6 p-8-12 mb-8 layout-bg lighter" | ||
| @mouseenter="showicon = true" |
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.
The code you provided contains several improvements that address potential issues and improve readability:
-
Nullish Coalescing Operator:
v-for="(item, index) in nodeModel.properties.config.chatFields || []"
This uses JavaScript's nullish coalescing operator (
||) to ensure thatnodeModel.properties.config.chatFieldsis treated as falsy if it's undefined ornull, preventing errors from attempting to iterate over a non-existent array. -
Conditional Check:
<template v-if="nodeModel.properties.config.chatFields?.length">
This adds a conditional check to verify that
chatFieldshas elements before rendering. It also uses optional chaining (??) to avoid accessing properties of an undefined or null object. -
Variable Initialization:
Although this doesn't explicitly solve a problem but improves clarity, initializing variables can help document the purpose of each variable:let showicon = false;
Here's the revised version of your code with these improvements:
<div>
<!-- Existing content -->
</div>
<template v-if="nodeModel.properties.config && nodeModel.properties.config.chatFields?.length">
<h5 class="title-decoration-1 mb-8">{{ $t('views.applicationWorkflow.variable.chat') }}</h5>
<div
v-for="(item, index) in nodeModel.properties.config.chatFields || []
:key="index"
class="flex-between border-r-6 p-8-12 mb-8 layout-bg lighter"
@mouseenter="showicon = true" @mouseleave="showicon = false"
>
<!-- Content for chat fields -->
</div>
</template>
<!-- Example additional line added to demonstrate better understanding -->
let showicon = false; // To store whether icon should be shown on hoverThese changes make the code more robust, easier to understand, and less prone to runtime errors caused by unexpected data structures.
style: MCP config dialog style