-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: Set display history #3937
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 |
| import { hasPermission } from '@/utils/permission/index' | ||
| const props = defineProps<{ | ||
| applicationDetail: any | ||
| chatLogData: any[] |
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.
This code appears to be modifying an Echarts-based component in a Vue application with multiple conditional statements related to whether or not the device is in desktop mode (isPcCollapse). There are no apparent syntax errors or logical issues but there might be some improvements on formatting readability, clarity, and performance.
Irregularities and Suggestions:
-
Duplicated Conditional Rendering:
<div v-show="!isPcCollapse" class="flex-between p-8 pb-0 color-secondary mt-8" v-if="applicationDetail.show_history || hasPermission(EditionConst.IS_CE, 'OR')" > <!-- Content here --> </div>
The condition
v-if="(applicationDetail.show_history || hasPermission(EditionConst.IS_CE, 'OR')) && !isPcCollapse"can be simplified using short-circuiting logic to avoid unnecessary evaluation ofhasPermission. -
Redundant Conditionality inside Components:
- In the
<common-list>and<el-scrollbar>components where conditions seem to control visibility based on the same variable or permissions, consider creating utility functions (likeshouldShowHistoryList) that encapsulate the logic instead of duplicating it across different areas.
- In the
-
Unused Variables/Functions:
- Ensure all used variables and functions have been declared (
ref(), import paths, etc.) before they're referenced within templates.
Remove unused imports (ApplicationIcon,AppTooltipetc.)
- Ensure all used variables and functions have been declared (
-
Code Readability & Formatting:
- Group imports together logically.
- Ensure consistent spacing around operators and keywords like equal signs (
=).
Avoid overly complex nested structures if possible, which improves readability.
Overall, this code should function correctly, however refining it would make it more maintainable and efficient.
<template>
<div v-show="!isPcCollapse">
<ElScrollbar when-scroll="auto" class="">
<CommonList
v-if="applicationDetail.show_history || shouldShowHistoryList()"
>
<!-- List content here -->
</ CommonList>
ElScrollbar>
<div class="" v-if="applicationDetail.show_history || shouldShowHistoryList()">
...
</ div>
</div>Make sure you replace shouldShowHistoryList() with either a helper method in the script section or directly evaluate the permission checks according to best practices.
| <img v-else :src="getImgUrl(item && item?.document_name)" alt="" width="24" /> | ||
| <div | ||
| class="ml-4 ellipsis-1" | ||
| :title="item?.document_name" |
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 provided code snippet has a few improvements to make it more efficient and robust:
- Use
v-showinstead ofv-ifin the conditional rendering to avoid unnecessary component re-renders whenitem?.document_namechanges but the source file ID/url remains empty.
<img
src="@/assets/fileType/web-link-icon.svg"
alt=""
width="24"
v-show="(item?.meta?.source_file_id || item?.meta?.source_url) && !isImageType()"
/>Add this at the top of your script or data section:
import { isImageType } from './utils'; // Import utility function if needed
// In your methods or computed properties
data() {
return {};
}
methods: {
isImageType() {
const imgTypes = ['jpg', 'jpeg', 'png', 'gif'];
return imgTypes.includes((this.item?.meta?.document_name || '').toLowerCase().split('.').pop());
},
},
...
</script>This change ensures that the image icon is not displayed unless either the document name is an image type or there's no associated file URL/source file ID. Also, remove unnecessary space between elements inside .flex-between.
| <AppIcon iconName="app-copy" class="color-secondary" /> | ||
| </el-button> | ||
| </el-form-item> | ||
| </el-form> |
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
@mouseenterand@mouseleaveevents are called whenever a mouse enters or leaves the element they are attached to. It seems that you intend to show an icon when either condition is met. - In both instances where the button has been wrapped with
<el-button>instead of just the closing bracket, there's still one more opening brace at the end. This doesn't pose an issue for HTML as long as it's balanced, but good practice is to have only one closing tag per line.- Suggestion: Ensure each closing tag stands on its own line to make the code cleaner.
<!-- Code after modifications -->
<cl<|fim_suffix|>ntent="">In summary:
- Review the event handlers for better readability (
mouseenter,mouseleave) ensuring they function correctly as intended. - Remove redundant braces around the elements.
- Maintain proper indentation for clarity.
feat: Set display history