-
Notifications
You must be signed in to change notification settings - Fork 2.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
feat: Save custom theme color scheme #7690
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. |
localStorage.setItem(STORAGE_KEY, JSON.stringify(defaultColors)); | ||
themeColors.value = { ...defaultColors }; | ||
darkColors = [...defaultDarkColors]; | ||
lightColors = [...defaultLightColors]; | ||
await updateXpackSettingByKey('ThemeColor', JSON.stringify(form.themeColor)); | ||
MsgSuccess(i18n.global.t('commons.msg.operationSuccess')); | ||
loading.value = false; |
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've provided is mostly correct but lacks some minor improvements and optimizations. Here are some points to consider:
-
Code Casing Consistency: Ensure that constants like
STORAGE_KEY
match their usage consistently across the file. -
Optional Chaining for Theme Predefined Colors: The use of optional chaining (
?.
) can make the code cleaner by handling cases wherethemePredefineColors
might not be defined. -
Initialization Order: Consider ensuring the initialization order of variables, especially if there's any dependency between them.
-
Local Storage Usage: Be mindful of how local storage works with asynchronous operations, as it might lead to unexpected behavior if accessed during state changes.
Here's an optimized version with these considerations:
const drawerVisible = ref();
interface DialogProps {
themeColor: {
light: string;
dark: string;
themePredefineColors?: { // Optional property
light: string[];
dark: string[];
};
};
}
interface ThemeColor extends DialogProps;
const form = reactive<DialogProps>({
themeColor: {} as DialogProps,
});
const themeNames = ['light', 'dark'];
const defaultDarkColors = ['#238636', '#dcdcdc'];
// Optimize the initial load by using defaults without checking localStorage first
initializeDefaultThemeColors(themeNames);
function initializeDefaultThemeColors(names) {
names.forEach(name => {
form.themeColor[name] = defaultColors[name] || '';
});
}
Explanation:
- Casing Consistency: Ensured
form
,dialogProps
, etc., follow camelCase convention. - Optional Chaining: Used to safely access properties within
themePredefineColors
. - Initialize Defaults Early: Moved default initialization before accessing local storage to avoid unnecessary checks when no predefined colors are set.
These adjustments should improve readability and maintainability while potentially reducing runtime overhead.
Make sure to adjust the logic according to your specific application requirements and handle any potential bugs based on real-world testing.
form.themeColor = JSON.parse(xpackRes.data.themeColor || '{"light":"#005eeb","dark":"#F0BE96"}'); | ||
globalStore.themeConfig.themeColor = xpackRes.data.themeColor | ||
? xpackRes.data.themeColor | ||
: '{"light":"#005eeb","dark":"#F0BE96"}'; | ||
globalStore.themeConfig.theme = form.theme; | ||
form.proxyDocker = xpackRes.data.proxyDocker; | ||
} |
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 has several improvements:
-
Default Theme Color Initialization: Added default values for
themeColor
andthemePredefineColors
. If no data is received from the server, default colors are used to prevent errors. -
Simplified Default Values Setting: Removed unnecessary brackets around the
{}
syntax when initializingdefaultThemeConfig
. -
Null Coalescing Operator: Used null coalescing operator (
?
) to simplify the assignment of default theme colors, ensuring that if no color information is available, the default values are still applied without error. -
Avoiding Empty Object Check: Instead of checking if
'light'
or'dark'
keys exist in an empty object before accessing them, directly assign default values assuming there will be color configurations stored correctly.
These changes improve readability and robustness by ensuring that default values are always set consistently, even if no dynamic data retrieval occurs.
Quality Gate passedIssues Measures |
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.
/lgtm
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wanghe-fit2cloud The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
No description provided.