-
Notifications
You must be signed in to change notification settings - Fork 2.3k
perf: Partial icon updates #3847
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. 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-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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
), | ||
]) | ||
}, | ||
}, | ||
'app-resource-management': { | ||
iconReader: () => { | ||
return h('i', [ |
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 snippet appears to be part of a Vue component's configuration file, likely used in Element Plus or Vaadin applications. Here’s a brief review:
Irregularities:
- Duplicate Icon Definition: The
iconReader
function is defined twice under different keys ('app-user-chat-active'
and'app-resource-management'
). If you intend these icons to have different functionality, consider consolidating them.
Potential Issues:
- SVG Syntax: There seems to be an issue with the SVG data. Ensure that it correctly defines the path attributes without any syntax errors.
Optimization Suggestions:
-
Consolidate Icons: Combine the two similar
iconReader
functions into one if they have different functionalities:'app-user-chat-active-and-management': { iconReader: () => { return h('i', [ h( 'svg', { style: { height: '100%', width: '100%' }, viewBox: '0 0 1024 1024', version: '1.1', xmlns: 'http://www.w3.org/2000/svg', }, [ // Existing paths... ], ), ]) }, },
-
Error Handling: Add basic error handling within the SVG definition to catch and display any rendering errors:
'app-user-chat-active-and-management': { iconReader: () => { try { return h('i', [ h( 'svg', { style: { height: '100%', width: '100%' }, viewBox: '0 0 1024 1024', version: '1.1', xmlns: 'http://www.w3.org/2000/svg', }, [ // Existing paths... ], ), ]); } catch (error) { console.error('Failed to render SVG:', error); return null; // Return a default content or element if needed } }, },
These changes will help ensure smooth rendering of both icons while maintaining consistency in your application’s design.
> | ||
</span> | ||
<span> {{ row.name }}</span> | ||
</el-space> | ||
</template> | ||
</el-table-column> | ||
<el-table-column |
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 has minor improvements to improve readability and user experience:
-
Use of
slot-scope
instead of#default
: While[#default]
is allowed, usingslot-scope="props"
explicitly can make the template more readable. This approach also allows access to additional properties like index in future versions.
-<template #default="{ row }">
+
-
<!-- Added comment to explain why this isn't necessary -->
2. **Consistent Styling:** The styles for the icons and text should remain consistent unless there's specific reason to change them. You might want to review the `.el-avatar` class or add a custom one if needed.
3. **Avoiding Inline HTML with Templates:** Directly assigning an icon via innerHTML within a Vue component is generally discouraged because it bypasses Vue's data binding system. If you need to render dynamic content, consider using Vue components, slots, mixins, etc., to ensure proper reactivity and maintainability.
```xml
- <span
- style="width: 24px; height: 24px; display: inline-block"
- :innerHTML="getRowProvider(row)?.icon"
- >
+ <div v-if="row.icon">
// Alternatively, use a custom avatar component
<my-custom-icon :icon="'user'" />
</div>
<span> {{ row.name }}</span>
```
Here is the modified part of the code following these suggestions:
```vue
<el-table-column width="240" :label="$t('common.name')" show-overflow-tooltip>
<template slot-scope="props">
<el-space :size="8">
<div v-if="getRowProvider(props.row)?.icon">
// Optionally replace 'my-custom-icon' with actual component name
<my-custom-icon :icon="'user'" />
</div>
<span> {{ props.row.name }} </span>
</el-space>
</template>
</el-table-column>
:deep(.el-checkbox__input.is-checked + .el-checkbox__label) { | ||
color: var(--el-checkbox-text-color); | ||
} | ||
} | ||
} | ||
</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 provided code snippet is already mostly correct, but you can add some minor optimizations:
<style>
/* Additions to optimize specificity */
.theme-form .el-checkbox__label:hover,
.theme-form .el-checkbox__inner:focus {
background-color: transparent;
}
.theme-form label.el-checkbox--mini+.el-checkbox__label{
font-size: 1rem; /* Apply consistent font size for all labels regardless of size */
}
</style>
Key Suggestions:
-
Specificity: The added rules have higher specificity than generic ones with
*
selectors. This ensures they take precedence over potentially less specific styles elsewhere. -
Hover and Focus Styles: Added hover and focus styles to
.el-checkbox__label
, which might be useful for enhancing user interaction experiences. -
Consistent Label Size: Applied a consistent font size (
1rem
) to.el-checkbox__label
, assuming this aligns with your design requirements or UI guidelines.
Make sure these changes do not conflict with existing styles in other parts of your application. Adjust the properties as necessary to fit your project's style guide or theme conventions.
perf: Partial icon updates