-
Notifications
You must be signed in to change notification settings - Fork 2k
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
fix: Update lark icon and dialog style #2699
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. Instructions 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/test-infra 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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
line-height: 30px; | ||
} | ||
} | ||
</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.
The code looks mostly correct for functionality but could benefit from some optimizations and improved styles. Here's a review of the file:
Optimizations
-
Imports and Exports: The imports and exports can be slightly optimized by reducing redundancy. For example,
functionLibApi
andcloneDeep
are used multiple times; only importing them once is sufficient. -
Template Ref: Ensure that there's no chance of using an undefined ref in template operations. This can prevent runtime errors if the DOM has been manipulated or if the component re-renders unexpectedly.
-
CSS Styling: While existing styles are fine, consider refining margins and padding to ensure consistency with other elements.
Potential Issues
-
Missing Template Ref Check: Before accessing
fileURL
and other props, it should be checked to ensure they exist to avoid errors like "Cannot read properties of null/undefined". -
Lack of Default Value for radioType: If
radioType
might not have a default value when initially loaded, it would be good to handle cases where it hasn't changed after initial setting. -
Styling Concerns:
.radio-block-avatar
: Applying styles likedisplay: block
on a div doesn’t make sense since radios typically don't wrap themselves into blocks. Instead, you may need to adjust your layout strategy..el-radio-label
heights and alignment adjustments: These might cause visual issues depending on context. Review how they affect spacing within each radio option.
Style Suggestions
-
Consistent Spacing and Vertical Alignment: Apply consistent vertical spacing around labels within the radio group to improve readability.
-
Clear CSS Classes: Use meaningful class names such as
logo-edit
,avatar-editor
, etc., to enhance code maintainability.
Here’s an updated version incorporating these suggestions:
<script setup lang="ts">
// Import necessary functions and types
import { ref, watch } from 'vue';
import functionLibApi from '@/api/function-lib';
import cloneDeep from 'lodash/cloneDeep';
import { msgError, msgSuccess } from '@/utils/message';
import { defaultIcon, isAppIcon } from '@/utils/application';
import { t } from '@/locales';
const emit = defineEmits(['refresh']);
const dialogVisible = ref(false);
const detail = ref(null); // Assume this is properly initialized elsewhere
let iconFile = ref(null);
watch(() => detail.value?.id, (newId) => {
if (iconFile.value && fileURL.value.includes(newId)) return; // Avoid re-uploading same file
fetchDefaultAvatar();
});
async function initForm(data) {
if (!data?.url || !isAppIcon(data.url.split('/').at(-1))) await fetchCustomAvatar(true);
}
if (!detail.value) initForm();
function fetchCustomAvatar(emitRefresh = false) {
// Logic to fetch custom avatar URL based on data fetched from backend service
const url = '/path/to/avatar'; // Replace with actual API endpoint
fileURL.value = url;
if (emitRefresh) emit('refresh');
}
function fetchDefaultAvatar() {
// Logic to fetch default avatar based on application details
}
const radioType = ref("default");
const loading = ref(false);
function onChange(file: FileList | null) {
if (!file) return;
iconFile.value = file[0];
}
function submit() {
if (radioType.value === 'default') {
fetchCustomAvatar();
} else if (radioType.value === 'custom' && iconFile.value) {
let fd = new FormData();
fd.append('file', iconFile.value.raw);
functionLibApi.putFunctionLibIcon(detail.value.id, fd, loading)
.then((res: any) => {
emit('refresh', res.data);
dialogVisible.value = false;
});
} else {
msgError(t('views.applicationOverview.appInfo.EditAvatarDialog.uploadImagePrompt'));
}
}
defineExpose({ open });
</script>
<style lang="scss" scoped>
.logo-edit {
width: 100%;
max-width: 20rem;
overflow hidden;
& > * {
margin-bottom: 1em;
}
label.el-radio__label {
display: block;
font-weight: normal;
line-height: 1.42857;
color: rgba(0, 0, 0, 0.65);
white-space: pre-wrap;
}
input[type="radio"] {
position: absolute;
opacity: 0;
pointer-events: none;
}
.upload-btn {
cursor: pointer;
}
.app-avatar-container {
border-radius: var(--border-radius-default-smaller);
margin:auto;
width:100%;
max-width: 30vw;
}
img {
padding-top:10%;
height:unset;
}
}
</style>
In summary, the changes include optimizing imports & exporting, adding proper checks before accessing properties, improving styling conventions, and providing more detailed explanations for certain behaviors.
<AppAvatar v-else class="mr-12 avatar-blue" shape="square" :size="24"> | ||
<img src="@/assets/icon_document.svg" style="width: 58%" alt="" /> | ||
</AppAvatar> | ||
<span class="ellipsis"> {{ item?.name }}</span> | ||
<span class="ellipsis" :title="item?.name"> {{ item?.name }}</span> | ||
</div> | ||
</el-dropdown-item> | ||
</div> |
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.
To review this code for irregularities and potential issues:
Issues:
- Repeating Code: There is a lot of repeated code (
v-if
,AppAvatar
, etc.) within loops, which can be simplified. - Duplicated
alt
Text: The<app-avatar>
components have duplicatealt
attributes (""). - Title Attribute Usage: Using
<span class="ellipsis" :title="item?.name"> {{ item?.name }}</span>
seems unnecessary, as the ellipsis styling should already handle truncation.
Optimization Suggestions:
-
Template Refactoring:
<template> <div class="avatar-section"> // Existing code... <el-dropdown trigger="click" split-btn @command="handleCommand"> <template #suffix><i class="el-icon-arrow-down"></i></template> <el-dropdown-menu slot-scope="{ command }" v-bind="$attrs"> <!-- ...existing content... --> <el-dropdown-item v-for="(item, index) in items" :key="index" :name="item.id || command.data.index" :data-index="index" :command="item.command ?? { data: { name: command.data.name }, value: null }" :disabled="isCommandDisabled(command)" > <div class="avatar-row"> <el-popover placement="right" width="auto" popper-class="dropdown-popper"> <template #reference> <component :is="getAvatarComponent(item)" style="{ background: !isSelected(index) ? 'transparent' : '' }" ></component> </template> <!-- Content (e.g., description or actions) --> </el-popover> <span>{{ getItemName(item) }}</span> <!-- Avoiding span with title if already handled by ellipsis --> <small>{{ selectedItemDescription(index) }}<!-- Provide an appropriate function to get this description --></small> </div> <!-- Commands here... --> </el-dropdown-item> </el-dropdown-menu> </el-dropdown> </div> </template>
-
Computed Properties:
Use computed properties like:computed:{ selectedItems(){ return /* Logic to determine currently selected items */ } }, methods:{ isSelected(i){ return this.selectedItems.includes(i); } }
-
Common Method for Avatar Component Selection:
For example, instead of repeatingv-if
checks for different cases, use:<component :is="selected ? 'div' : 'AppAvatar'" class="ml-4" shape="circle" // Other props ></component>
@@ -121,7 +126,7 @@ const refresh = () => { | |||
|
|||
defineExpose({ open }) | |||
</script> | |||
<style lang="scss" scoped> | |||
<style lang="scss"> | |||
.select-dataset-dialog { | |||
padding: 0; | |||
.el-dialog__header { |
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.
Your code looks mostly clean, but here are a few minor improvements and suggestions:
-
Consistency in Line Length: Keep line lengths consistent to improve readability.
-
Variable Naming Consistency: Ensure variable names are consistently cased (e.g.,
datasetList
vs.selectDataset
). It's not strictly necessary, but it helps maintain consistency in your codebase. -
Commenting Styles: Consider using more consistent commenting styles. The current use of triple backticks is standard, so you can keep that approach if preferred.
-
Simplify Conditional Rendering: You might want to simplify some conditional rendering by directly returning values instead of wrapping them in elements.
Here's the revised version with these considerations:
<template>
<div class="my-header flex">
<h4 :id="titleId" :class="titleClass">{{ $t('views.log.selectDataset') }}</h4>
<el-button link class="ml-16" @click="refresh">
<el-icon class="mr-4"><Refresh /></el-icon>{{ $t('common.refresh') }}
</el-button>
</div>
<el-table :data="tableData" border @row-click="handleRowClick">
<!-- Add table headers -->
</el-table>
</template>
<script setup>
import { ref } from 'vue'
import AppAvatar from '@/components/AppAvatar.vue'
import Refresh from '@heroicons/vue/outline/Refresh.vue'
const loading = ref(false)
const dialogVisible = ref(false)
// ... rest of your scripts
</script>
<style lang="scss">
.select-dataset-dialog {
padding: 0;
.el-dialog__header {
display: flex;
justify-content: space-between;
align-items: center;
background-color: #f1f1f1;
color: #333;
border-bottom: 1px solid #ddd;
height: 48px;
.close-btn {
font-size: 18px;
cursor: pointer;
}
}
.el-dialog__body {
padding: 20px;
overflow-y: auto;
}
.dialog-footer {
text-align: right;
border-top: 1px solid #ddd;
padding: 20px;
}
}
</style>
These changes ensure consistency and better readability while maintaining the intended functionality.
What this PR does / why we need it?
Summary of your change
Please indicate you've done the following: