feat(theme): component style && common theme #814
Conversation
WalkthroughThe changes in this pull request focus on updating the CSS styling across various Vue components and LESS files. The updates involve modifying color variables to enhance the visual consistency of the application. Key alterations include the introduction of new CSS custom properties, the removal of outdated styles, and adjustments to existing styles to align with a new theming approach. These changes affect both light and dark themes, ensuring a cohesive user interface. Changes
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Outside diff range and nitpick comments (14)
packages/theme/base/src/dark/dark-common.js (1)
1-1: Consider using English for commentsThe comment is currently in Chinese. For better maintainability and to follow international coding standards, consider using English for all comments in the codebase.
Here's a suggested translation:
-// tinyvue组件的暗色主题 +// Dark theme for tinyvue componentspackages/theme/base/src/light/light-common.js (1)
1-4: Enhance documentation with more comprehensive commentsThe current comment is brief and in Chinese. Consider adding more detailed comments in English to improve accessibility for a wider range of developers.
Here's a suggestion for more comprehensive comments:
/** * Light theme for tinyvue components. * This theme defines the color palette and styles for the light mode of the application. * It includes functional colors for various UI states and components. */ export const tinyLightTheme = { // Functional colors // TODO: Add color definitions for primary, secondary, success, warning, error, etc. }packages/theme/base/src/var-component.less (2)
1-4: LGTM! Consider adding a comment for clarity.The progress bar CSS variables are well-defined and use appropriate semantic naming. The use of CSS variables promotes maintainability and theme consistency.
Consider adding a brief comment explaining the purpose of this mixin, for example:
// Define CSS variables for the progress bar component .component-css-vars-progress() { // ... existing code ... }
1-15: Overall recommendations for improving theme variablesWhile the introduction of CSS variables for these components is a step in the right direction for maintainability and consistency, there are several areas that need attention:
- Resolve all design uncertainties indicated by comments.
- Ensure consistent use of theme variables instead of hard-coded color values.
- Translate all comments to English for better collaboration.
- Add brief explanatory comments for each mixin.
Additionally, consider the following suggestions to enhance the theme system:
- Create a comprehensive color palette with semantic naming in a separate file, if not already present.
- Use a consistent naming convention for all variables, preferably following a structure like
--{component}-{element}-{property}-{state}.- Consider adding a light and dark theme toggle to test these variables in different contexts.
To improve the overall architecture of the theming system, consider implementing a theme generator that can create variations of these variables based on a set of base colors. This would allow for easier creation of multiple themes or color schemes in the future.
packages/theme/base/package.json (1)
25-25: Consider removing the empty "dependencies" object.An empty "dependencies" object has been added to the package.json file. While this doesn't cause any issues, it's generally unnecessary to include an empty dependencies object. It's typically better to add dependencies only when they are actually needed.
Unless there's a specific reason for including this empty object (e.g., placeholder for imminent dependencies), consider removing it:
- "dependencies": {},If you're planning to add dependencies soon, it would be helpful to mention this in a comment or in the PR description to provide context for this change.
packages/theme/base/src/index.js (1)
23-25: Excellent addition of defaultThemeList, consider future extensibility.The introduction of
defaultThemeListis a great improvement, providing a centralized object for accessing both light and dark themes. This aligns well with the PR objective of establishing a common theme structure.For future extensibility, you might consider using a more dynamic approach to populate this object. For example:
export const defaultThemeList = { [tinyThemeLightVars.id]: tinyThemeLightVars, [tinyThemeDarkVars.id]: tinyThemeDarkVars };This would allow for easier addition of new themes in the future, as the keys would be dynamically set based on the theme IDs.
packages/theme/base/src/page/base-config-page.less (2)
6-35: Scrollbar styles look great, consider using CSS variables for colors.The new scrollbar styles are well-implemented and provide a consistent look across the application. The use of classes for different scrollbar sizes offers flexibility.
Consider using CSS variables for the scrollbar colors to improve maintainability and theme consistency. For example:
:root { --scrollbar-thumb-color: #dbdbdb; --scrollbar-thumb-hover-color: #c2c2c2; } // Then in your scrollbar styles: &::-webkit-scrollbar-thumb { background-color: var(--scrollbar-thumb-color); &:hover { background-color: var(--scrollbar-thumb-hover-color); } }This change would make it easier to update colors across themes and improve overall consistency.
123-133: Great addition of utility classes, consider cross-browser compatibility for text-ellipsis-multiple.The new global classes are valuable additions. The use of a CSS variable for text color in
.global-desc-infois excellent for theming flexibility.For
.text-ellipsis-multiple, consider using a more cross-browser compatible solution. The current implementation uses-webkitprefixed properties, which may not work in all browsers. Here's an alternative that works across more browsers:.text-ellipsis-multiple { display: -webkit-box; -webkit-box-orient: vertical; -webkit-line-clamp: 2; overflow: hidden; text-overflow: ellipsis; /* Fallback for non-WebKit browsers */ display: block; height: 2.4em; /* Adjust based on your line-height */ line-height: 1.2em; }This solution provides a fallback for browsers that don't support
-webkit-line-clamp. Adjust theheightandline-heightvalues to match your design requirements.packages/design-core/src/init.js (3)
20-20: LGTM! Consider updating documentation.The change from importing
tinyEngineThemeLighttodefaultThemeListsuggests a more flexible theme management system, which aligns well with the PR's objectives. This modification allows for dynamic theme selection, enhancing the application's theming capabilities.Consider updating the documentation to reflect this change in the theming system, explaining how to use and configure the
defaultThemeList.
54-57: LGTM! Consider adding error handling.The new theme initialization logic effectively implements a more dynamic theme selection process, aligning well with the PR objectives. The use of
newRegistry.config.themefor configuration-based selection and setting thedata-themeattribute on the root element are good practices.Consider adding error handling for cases where the selected theme is not present in
defaultThemeList. For example:const theme = newRegistry.config.theme || 'light' if (defaultThemeList[theme]) { new TinyThemeTool(defaultThemeList[theme], defaultThemeList[theme]?.id) document.documentElement?.setAttribute?.('data-theme', theme) } else { console.warn(`Theme "${theme}" not found in defaultThemeList. Falling back to light theme.`) new TinyThemeTool(defaultThemeList['light'], defaultThemeList['light']?.id) document.documentElement?.setAttribute?.('data-theme', 'light') }
53-53: Remove commented out code.The commented out import statement for theme loading is no longer necessary due to the new theme initialization logic. To maintain code cleanliness and prevent confusion, it's best to remove this line entirely.
Please remove the following commented out code:
- // import(`./theme/${newRegistry.config.theme}.js`)packages/theme/base/src/common.less (1)
1-96: Consider adding English translations for comments to improve accessibilityThe comments throughout the file are written in Chinese. To enhance collaboration with international team members and improve maintainability, consider providing English translations or bilingual comments.
packages/theme/base/src/base.less (1)
444-456: Ensure Consistent Comment Formatting for Better ReadabilityIn the line-height section (lines 444-456), there is an inconsistency in comment styles:
- Lines 444-446 use
/** */block comments.- Line 447 uses
//for a single-line comment.For consistency and better readability, consider using a uniform comment style throughout the file.
packages/theme/base/src/component-common.less (1)
488-488: Address pending '待确认' comments before mergingThe code contains several '待确认' (to be confirmed) comments indicating that certain styles or values need verification. Please review and finalize these items to ensure the styles are correctly applied.
Lines with '待确认' comments:
- Line 488:
background-color: var(--te-common-bg-primary-checked); // 待确认,原来： --te-common-bg-primary- Line 512:
border: 1px solid var(--te-common-border-hover); // 待确认- Line 731:
background-color: var(--te-common-border-divider); // 待确认- Line 735:
color: var(--te-common-text-primary); // 待确认- Line 743:
background-color: var(--te-common-bg-disabled); // 待确认是用背景色 还是线条色- Line 755:
color: var(--te-common-text-primary); // 待确认Do you need assistance in deciding these values, or would you like me to open a GitHub issue to track these tasks?
Also applies to: 512-512, 731-731, 735-735, 743-743, 755-755
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (40)
- packages/design-core/src/init.js (2 hunks)
- packages/theme/base/package.json (1 hunks)
- packages/theme/base/src/base.js (0 hunks)
- packages/theme/base/src/base.less (1 hunks)
- packages/theme/base/src/common.less (1 hunks)
- packages/theme/base/src/component-common.less (1 hunks)
- packages/theme/base/src/dark-component.js (0 hunks)
- packages/theme/base/src/dark/dark-common.js (1 hunks)
- packages/theme/base/src/index.js (1 hunks)
- packages/theme/base/src/light-component.js (0 hunks)
- packages/theme/base/src/light/light-common.js (1 hunks)
- packages/theme/base/src/page/base-config-page.less (4 hunks)
- packages/theme/base/src/var-component.less (1 hunks)
- packages/theme/common/global.less (0 hunks)
- packages/theme/dark/index.less (0 hunks)
- packages/theme/dark/radio.less (0 hunks)
- packages/theme/dark/tiny-checkbox.less (0 hunks)
- packages/theme/dark/tiny-collapse.less (0 hunks)
- packages/theme/dark/tiny-dialog-box.less (0 hunks)
- packages/theme/dark/tiny-grid.less (0 hunks)
- packages/theme/dark/tiny-input.less (0 hunks)
- packages/theme/dark/tiny-modal.less (0 hunks)
- packages/theme/dark/tiny-notify.less (0 hunks)
- packages/theme/dark/tiny-numeric.less (0 hunks)
- packages/theme/dark/tiny-search.less (0 hunks)
- packages/theme/dark/tiny-switch.less (0 hunks)
- packages/theme/dark/tiny-tabs.less (0 hunks)
- packages/theme/light/canvas.less (1 hunks)
- packages/theme/light/index.less (0 hunks)
- packages/theme/light/radio.less (0 hunks)
- packages/theme/light/settings.less (1 hunks)
- packages/theme/light/tiny-collapse.less (0 hunks)
- packages/theme/light/tiny-dialog-box.less (0 hunks)
- packages/theme/light/tiny-grid.less (0 hunks)
- packages/theme/light/tiny-input.less (0 hunks)
- packages/theme/light/tiny-modal.less (0 hunks)
- packages/theme/light/tiny-notify.less (0 hunks)
- packages/theme/light/tiny-numeric.less (0 hunks)
- packages/theme/light/tiny-search.less (0 hunks)
- packages/theme/light/tiny-switch.less (0 hunks)
Files not reviewed due to no reviewable changes (28)
- packages/theme/base/src/base.js
- packages/theme/base/src/dark-component.js
- packages/theme/base/src/light-component.js
- packages/theme/common/global.less
- packages/theme/dark/index.less
- packages/theme/dark/radio.less
- packages/theme/dark/tiny-checkbox.less
- packages/theme/dark/tiny-collapse.less
- packages/theme/dark/tiny-dialog-box.less
- packages/theme/dark/tiny-grid.less
- packages/theme/dark/tiny-input.less
- packages/theme/dark/tiny-modal.less
- packages/theme/dark/tiny-notify.less
- packages/theme/dark/tiny-numeric.less
- packages/theme/dark/tiny-search.less
- packages/theme/dark/tiny-switch.less
- packages/theme/dark/tiny-tabs.less
- packages/theme/light/index.less
- packages/theme/light/radio.less
- packages/theme/light/tiny-collapse.less
- packages/theme/light/tiny-dialog-box.less
- packages/theme/light/tiny-grid.less
- packages/theme/light/tiny-input.less
- packages/theme/light/tiny-modal.less
- packages/theme/light/tiny-notify.less
- packages/theme/light/tiny-numeric.less
- packages/theme/light/tiny-search.less
- packages/theme/light/tiny-switch.less
Additional comments not posted (10)
packages/theme/base/src/dark/dark-common.js (1)
1-4: 🛠️ Refactor suggestionConsider adding type information and clarifying theme structure
To improve maintainability and developer experience, consider the following suggestions:
- Add type information to the
tinyDarkThemeobject. This could be done using TypeScript or JSDoc comments.- Clarify how this common dark theme relates to other theme files or customizations in the project.
Here's an example of how you might add type information using JSDoc:
/** * @typedef {Object} DarkTheme * @property {string} primaryColor - The primary color for the dark theme * @property {string} backgroundColor - The background color for the dark theme * @property {string} textColor - The text color for the dark theme * // Add more properties as needed */ /** * Dark theme for tinyvue components * @type {DarkTheme} */ export const tinyDarkTheme = { // Functional colors // Implement properties here }To better understand the theme structure in the project, let's check for other theme-related files:
This will help us understand how
dark-common.jsfits into the overall theming system.packages/theme/base/src/light/light-common.js (1)
1-4: Clarify the overall theming strategyThe file name and structure suggest this is part of a larger theming system. It would be helpful to understand how this file fits into the overall strategy.
Could you provide more context on the theming approach?
- Is there a corresponding dark theme?
- How are these themes applied to components?
- Are there plans to expand this theme with more properties beyond colors?
To help verify the theming strategy, we can run the following script:
This will help us understand the broader context of the theming system and how this light theme is intended to be used.
Verification successful
Clarification on Theming Strategy
The theming system includes both light and dark themes. The
tinyLightThemedefined inlight-common.jsis complemented bytinyDarkThemeindark-common.js. These themes are centralized inpackages/theme/base/src/index.js, where they are imported and applied to components. Currently, the themes focus on color properties, with potential plans to expand to additional properties in the future.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for related theme files and usage echo "Searching for related theme files:" fd -e js -e ts theme echo "\nSearching for theme usage in components:" rg --type js --type ts "tinyLightTheme" -C 3Length of output: 1303
packages/theme/base/src/index.js (2)
9-21: Theme variable exports look good, but clarify the 'cnName' property.The structure and content of the
tinyThemeLightVarsandtinyThemeDarkVarsexports are well-defined and consistent. The use of the spread operator to include all properties from the imported theme objects is a good practice.However, I noticed that the 'cnName' property is empty for both themes. Is this intentional, or should it contain a Chinese name for the themes?
1-7: Verify the necessity of all imports.The file imports several LESS files and theme modules. While this provides a comprehensive set of styles and themes, it's important to ensure that all these imports are necessary and used within this file or its dependencies.
To verify the usage of these imports, you can run the following script:
If any of these imports are unused, consider removing them to keep the file lean and improve performance.
Verification successful
All imports are necessary and correctly used.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for unused imports in the file # Test: Search for usage of imported LESS files and theme modules rg --type js -i -e "base-config-page" -e "base\.less" -e "common\.less" -e "component-common\.less" -e "tinyDarkTheme" -e "tinyLightTheme" packages/theme/base/srcLength of output: 971
packages/theme/light/canvas.less (4)
7-11: LGTM! Consider updating related documentation.The updates to the custom properties for text colors and corner mark styles are consistent with the new naming convention. The use of more specific color variables (e.g.,
--te-common-text-dark-inverse) could lead to better consistency across different themes.To ensure that these changes are reflected in any related documentation, please run the following script:
#!/bin/bash # Description: Check for any documentation files that might need updating rg --type md 'ti-lowcode-canvas' docs/If any results are found, consider updating the documentation to reflect the new variable names.
22-24: LGTM! Check for related menu components.The updates to the custom properties for menu item colors and hover states are consistent with the previous changes. The use of more generic color variables (e.g.,
--te-common-text-primary) could improve consistency across different components.To ensure that these changes don't affect other menu components unintentionally, please run the following command to find any files related to menus:
#!/bin/bash # Description: Find files related to menu components rg --type less 'menu' packages/Review the results to check if any other menu components need similar updates or if they might be affected by these changes.
16-21:⚠️ Potential issueConsider completing the refactoring for all properties.
Some custom properties still use the old
--ti-lowcode-prefix, while others have been updated to the new--te-prefix. This inconsistency could lead to confusion and maintenance issues in the future.To maintain consistency, consider updating the following properties to use the new prefix:
- --ti-lowcode-canvas-footer-border-top-color: var(--ti-lowcode-common-border-color-4); - --ti-lowcode-canvas-tab-handle-bg: var(--ti-lowcode-common-secondary-text-color); - --ti-lowcode-canvas-tab-handle-hover-bg: var(--ti-lowcode-common-primary-color); - --ti-lowcode-canvas-tab-handle-color: var(--ti-lowcode-common-text-color-2); - --ti-lowcode-canvas-menu-bg: var(--ti-lowcode-common-component-bg); - --ti-lowcode-canvas-menu-item-disabled-color: var(--ti-lowcode-base-text-color-4); + --te-canvas-footer-border-top-color: var(--te-common-border-default); + --te-canvas-tab-handle-bg: var(--te-common-text-secondary); + --te-canvas-tab-handle-hover-bg: var(--te-common-color-primary); + --te-canvas-tab-handle-color: var(--te-common-text-secondary); + --te-canvas-menu-bg: var(--te-common-bg-container); + --te-canvas-menu-item-disabled-color: var(--te-common-text-disabled);Please note that the suggested variable names are based on the naming convention observed in the updated properties. You may need to adjust them according to your specific design system.
To ensure all properties are updated consistently, run the following command:
#!/bin/bash # Description: Check for any remaining --ti-lowcode- prefixed properties in the file rg --type less '\-\-ti-lowcode-' packages/theme/light/canvas.lessIf any results are found, consider updating them to use the new prefix.
Also applies to: 25-25
12-15: LGTM! Suggest visual inspection of updated styles.The updates to the custom properties for corner mark styles are consistent with the previous changes. The use of more specific background color variables (e.g.,
--te-common-bg-primary-checked) suggests a more standardized approach to styling checked or selected states.To ensure that these changes don't introduce any unexpected visual changes, please run the following command to find any test files related to the canvas component:
#!/bin/bash # Description: Find test files related to the canvas component fd -e test.js -e spec.js canvas packages/If any test files are found, consider running visual regression tests or manually inspecting the component in different states to verify the new styles.
packages/design-core/src/init.js (1)
Line range hint
1-89: Overall, great improvements to theme management!The changes in this file significantly enhance the theme management system, allowing for more flexible and dynamic theme selection. This aligns perfectly with the PR's objectives of improving component styling and implementing a common theme. The new implementation provides a solid foundation for future theme-related features and customizations.
Key improvements:
- More flexible theme import system
- Dynamic theme selection based on configuration
- Proper theme initialization and application
These changes will greatly benefit the project's theming capabilities and user experience.
packages/theme/base/src/base.less (1)
350-351: Verify Utilization of Company Brand Color VariableThe variable
--te-base-brand-coloris defined (line 350) but it's unclear if and where it's utilized in the theme. Ensure that this brand color is applied appropriately across components that should reflect the company's branding.
|
|
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (10)
packages/theme/light/toolbar.less (1)
Line range hint
1-19: Suggestions for improving color management and consistencyWhile reviewing the file, I noticed a few areas that could be improved:
Hardcoded color values: Some properties use hardcoded RGB values (e.g.,
rgb(235, 235, 235)). Consider replacing these with CSS custom properties for better maintainability and consistency.Transparency handling: The comment on line 10 indicates a specific transparency calculation. Instead of using a hardcoded RGB value, consider using
rgba()or a CSS color function that allows for alpha channel manipulation.Naming consistency: There's a mix of
--ti-lowcode-and--te-prefixes for variables. It might be beneficial to standardize these prefixes for better organization and clarity.Here's an example of how you could refactor some of these issues:
:root { --te-common-gray-95: rgb(235, 235, 235); --te-common-gray-96: rgb(240, 240, 240); } .tiny-engine-toolbar { // ... other properties ... --ti-lowcode-toolbar-hover-color: var(--te-common-bg-container); --ti-lowcode-toolbar-view-active-bg: rgba(25, 25, 25, 0.05); // 5% opacity of #191919 --ti-lowcode-toolbar-left-icon-bg-hover: var(--te-common-gray-96); // ... other properties ... }This approach:
- Introduces new common color variables
- Uses
rgba()for transparency- Maintains consistency in naming and usage of variables
Consider applying similar principles throughout the file to improve overall maintainability and consistency.
packages/layout/src/DesignSettings.vue (2)
73-74: Enhanced tab item styling and layoutThe addition of
flex: 1to the.tiny-tabs__itemclass ensures equal distribution of space among tab items, improving the overall layout. The use of a CSS variable for the background color supports consistent theming across the application.Consider adding a transition property for smooth color changes when hovering or activating tabs:
.tiny-tabs__item { flex: 1; background-color: var(--ti-lowcode-setting-panel-bg-color); color: var(--ti-lowcode-setting-panel-tabs-item-title-color); + transition: color 0.3s ease; }
61-90: Overall improvement in tab component styling and themingThe changes made to the
DesignSettings.vuefile significantly enhance the styling and layout of the tab component. Key improvements include:
- Refined active tab indicator
- Improved tab item layout with flex properties
- Consistent use of CSS variables for colors, supporting theming
- Minor adjustments to padding and spacing for better visual appeal
These changes align well with the PR objectives of enhancing component styling and implementing a common theme. The use of CSS variables promotes consistency and facilitates future theme modifications.
To further improve the component's flexibility and reusability, consider extracting the tab-specific styles into a separate, reusable component or applying them globally if these styles are meant to be used across multiple components. This would enhance the modularity of your codebase and make it easier to maintain consistent styling across the application.
packages/theme/light/block.less (1)
30-30: Approved with a minor observation on naming convention.The update to
--ti-lowcode-component-block-version-list-desc-coloraligns with the goal of enhancing visual consistency across the application. The new valuevar(--te-common-text-weaken)appropriately describes its purpose for weakened text, suitable for descriptions in a version list.Consider maintaining consistency in variable naming conventions. The new variable uses a "te-" prefix, while other variables in this file use "ti-lowcode-". If this is intentional as part of a broader refactoring, please ensure it's documented. Otherwise, consider aligning the prefix with the existing convention.
packages/configurator/src/slot-configurator/SlotConfigurator.vue (1)
Line range hint
1-201: Enhance accessibility and internationalizationWhile the changes in this PR focus on styling, I noticed some areas where the component could be improved in terms of accessibility and internationalization:
Accessibility:
- Add
aria-labeloraria-labelledbyattributes to the switch elements to provide better context for screen readers.- Ensure that the color contrast ratio meets WCAG guidelines, especially with the new styling changes.
Internationalization:
- Consider using a translation function (e.g.,
$t) for hardcoded strings like "提示" and "关闭后插槽内的内容将被清空，是否继续？" to support multiple languages.Here's an example of how you could improve the switch element:
<div :class="['e__switch', { 'e_is-checked': slot.bind }]" role="switch" :aria-checked="slot.bind" :aria-label="$t('toggle_slot', { name: slot.label })" > <span class="e__switch-core" @click="toggleSlot(slot, index)"></span> </div>And for the confirmation modal:
useModal().confirm({ title: $t('confirmation.title'), message: $t('confirmation.close_slot_message'), // ... rest of the code });These changes would make the component more accessible and easier to localize in the future.
packages/settings/styles/src/components/background/LinearGradient.vue (2)
82-84: Good move, but consider using dynamic colors.The
angleChangefunction has been appropriately moved before its usage, improving code readability. However, the gradient colors are hardcoded to black and white.Consider using the colors from
state.colorListinstead of hardcoding them:const angleChange = () => { - updateStyle({ [BACKGROUND_PROPERTY.BackgroundImage]: `linear-gradient(${state.angle}deg, black, white)` }) + const colorStops = state.colorList.map(({color, percent}) => `${color} ${percent}`).join(', ') + updateStyle({ [BACKGROUND_PROPERTY.BackgroundImage]: `linear-gradient(${state.angle}deg, ${colorStops})` }) }This change would make the gradient colors dynamic and consistent with the rest of the component's behavior.
145-145: Approved: Simplified color matching regex.The modification to use
#instead of\#in the color matching regular expression is correct and simplifies the pattern.For consistency, consider using template literals for all regex patterns in this section. This would improve readability, especially for complex patterns:
const rColor = /#(?:[a-f0-9]{6}|[a-f0-9]{3})/ const rLengthPercentage = /(?:[+-]?\d*\.?\d+)(?:%|[a-z]+)?/ const rLinearColorStop = new RegExp(`${rColor.source}(?:\\s+${rLengthPercentage.source})?`) const rComma = /\s*,\s*/ const rColorStopList = new RegExp(`(?:(${rLinearColorStop.source}${rComma.source})*${rLinearColorStop.source})`, 'gi')packages/settings/styles/src/components/background/BackgroundImageSetting.vue (2)
172-173: Approved: Improved color variable naming.The changes to the
.row-contentclass improve consistency by using standardized color variables. This is a good practice for maintaining a cohesive theme across the application.Consider adding a comment explaining the purpose of these color choices, especially if they differ significantly from the previous values. This can help other developers understand the design decisions.
Line range hint
172-203: Summary: Improved theme consistency with minor UX concernThe changes in this file successfully implement the new theming approach mentioned in the PR objectives. The use of standardized color variables enhances the visual consistency of the application and improves maintainability. This aligns well with the PR's goal of introducing new features related to component styling and a common theme.
However, there's a potential usability issue with the similarity between hover and selected state backgrounds. Addressing this would further improve the user experience and align with the PR's emphasis on self-validation by the designer.
Regarding the comment by hexqi about a JavaScript UI problem, this file doesn't contain any JavaScript changes that could directly cause such an issue. However, the styling changes might indirectly affect the UI's appearance or behavior. It would be helpful to have more specific information about the reported problem to determine if it's related to these changes.
To ensure a comprehensive review of the theme changes:
- Verify that these color variable changes are consistent across all components affected by this PR.
- Consider creating a theme documentation or style guide that explains the usage of these new color variables to maintain consistency in future development.
- If not already done, implement a visual regression testing process to catch any unintended UI changes resulting from these theme updates.
packages/common/component/MetaCodeEditor.vue (1)
Line range hint
1-374: Summary: Theme updates require attention and consistency.The changes in this file are part of a broader theme update, but there are a couple of issues that need to be addressed:
- The color variable for
.empty-colorhas been updated correctly.- Both
.header-tips-titleandcodeelement color properties are using invalid emptyvar()functions.Additionally, it's recommended to conduct a comprehensive review of color variable usage across the project to ensure consistency with the new theming approach.
Consider creating a central theme file or using a design system to manage color variables consistently across the project. This would help prevent issues like the ones found in this file and make future theme updates easier to manage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (32)
- packages/common/component/ConfigItem.vue (1 hunks)
- packages/common/component/MetaCodeEditor.vue (3 hunks)
- packages/configurator/src/js-slot-configurator/JsSlotConfigurator.vue (1 hunks)
- packages/configurator/src/slot-configurator/SlotConfigurator.vue (1 hunks)
- packages/layout/src/DesignSettings.vue (1 hunks)
- packages/plugins/block/src/BlockEventList.vue (1 hunks)
- packages/plugins/block/src/BlockPropertyList.vue (1 hunks)
- packages/plugins/datasource/src/DataSourceRemoteDataResult.vue (1 hunks)
- packages/plugins/materials/src/meta/block/src/BlockGroup.vue (0 hunks)
- packages/settings/styles/src/components/background/BackgroundImageSetting.vue (2 hunks)
- packages/settings/styles/src/components/background/LinearGradient.vue (2 hunks)
- packages/settings/styles/src/components/spacing/SpacingGroup.vue (0 hunks)
- packages/settings/styles/src/components/typography/TypographyMore.vue (0 hunks)
- packages/theme/base/src/common.less (1 hunks)
- packages/theme/base/src/component-common.less (1 hunks)
- packages/theme/base/src/page/base-config-page.less (4 hunks)
- packages/theme/dark/button.less (0 hunks)
- packages/theme/dark/canvas.less (2 hunks)
- packages/theme/dark/index.less (1 hunks)
- packages/theme/dark/metaComponent.less (1 hunks)
- packages/theme/dark/settings.less (1 hunks)
- packages/theme/dark/tutorial.less (1 hunks)
- packages/theme/dark/variable.less (0 hunks)
- packages/theme/light/block.less (1 hunks)
- packages/theme/light/button.less (0 hunks)
- packages/theme/light/index.less (1 hunks)
- packages/theme/light/materials.less (1 hunks)
- packages/theme/light/metaComponent.less (1 hunks)
- packages/theme/light/settings.less (2 hunks)
- packages/theme/light/toolbar.less (1 hunks)
- packages/theme/light/tutorial.less (1 hunks)
- packages/theme/light/variable.less (0 hunks)
💤 Files not reviewed due to no reviewable changes (7)
- packages/plugins/materials/src/meta/block/src/BlockGroup.vue
- packages/settings/styles/src/components/spacing/SpacingGroup.vue
- packages/settings/styles/src/components/typography/TypographyMore.vue
- packages/theme/dark/button.less
- packages/theme/dark/variable.less
- packages/theme/light/button.less
- packages/theme/light/variable.less
✅ Files skipped from review due to trivial changes (1)
- packages/plugins/block/src/BlockEventList.vue
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/theme/base/src/common.less
- packages/theme/base/src/component-common.less
- packages/theme/base/src/page/base-config-page.less
- packages/theme/dark/index.less
- packages/theme/light/index.less
- packages/theme/light/settings.less
🔇 Additional comments not posted (19)
packages/theme/dark/tutorial.less (1)
4-4: LGTM. Verify visual consistency and accessibility.The update to
--ti-lowcode-tutorial-hover-bg-colorappears to be part of a larger effort to standardize color usage across the application. This change is likely to improve overall theme consistency.To ensure the change doesn't introduce any issues:
- Please verify that the new hover background color (
--te-common-bg-container) provides sufficient contrast with the text color for accessibility.- Check that this change maintains visual consistency with other components in the application.
- If there's any documentation describing the theming system or color usage, please update it to reflect this change.
You can use the following script to check for other occurrences of the old variable and ensure all have been updated:
This will help ensure that the theming update has been applied consistently across the project.
packages/theme/light/tutorial.less (1)
4-4: LGTM. Please verify the new variable and consider documenting the change.The update to
--ti-lowcode-tutorial-hover-bg-colorappears to be part of a larger effort to standardize theming across the application. This change fromvar(--ti-lowcode-base-bg)tovar(--te-common-bg-container)is likely intentional and aligns with the PR's objective of updating component styling and common theme.To ensure this change doesn't introduce any unintended side effects:
- Please verify that
--te-common-bg-containeris defined and has the expected value in your common theme files.- Check the visual impact of this change on the tutorial component's hover state in both light and dark themes.
Run the following script to verify the existence and usage of the new variable:
Consider adding a comment explaining the reason for this change, especially if it's part of a larger refactoring effort. This will help maintain context for future developers. For example:
/* Updated to use common container background as part of theme standardization effort */ --ti-lowcode-tutorial-hover-bg-color: var(--te-common-bg-container);packages/theme/light/toolbar.less (1)
8-8: Verify the impact of the toolbar hover color changeThe hover color for the toolbar has been updated from a lowcode-specific background color to a more general container background color. This change could be part of a broader effort to standardize colors across the application.
While this change may improve consistency, please ensure that:
- The new color provides sufficient contrast for accessibility.
- The hover state remains visually distinct from the default state.
- The change aligns with the overall design system and doesn't create inconsistencies with other components.
To verify the impact of this change, you can run the following commands:
These commands will help you identify any other components that might be affected by this change and ensure consistent usage of the new variable.
✅ Verification successful
Toolbar hover color change verified and approved
The change from
--ti-lowcode-base-bgto--te-common-bg-containerfor the toolbar hover color has been verified. This modification is part of a broader effort to standardize colors across the application, which should improve overall visual consistency.Key points:
- The new variable
--te-common-bg-containeris used correctly in the toolbar.less file.- This variable is defined differently for light and dark themes, ensuring proper theme support.
- The same variable is used in multiple components, promoting consistency throughout the application.
While we cannot directly assess the visual impact or accessibility without seeing the rendered result, the change appears to be well-integrated into the existing color system. However, it's recommended to:
- Visually verify the hover state remains distinct from the default state in both light and dark themes.
- Conduct an accessibility check to ensure sufficient contrast with text and other UI elements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other occurrences of the old variable echo "Checking for other occurrences of --ti-lowcode-base-bg:" rg --type less "--ti-lowcode-base-bg" # Check for the usage of the new variable echo "Checking for occurrences of --te-common-bg-container:" rg --type less "--te-common-bg-container" # Check for any potential color contrast issues echo "Checking for potential color contrast issues:" rg --type less "color:|background:|background-color:"Length of output: 99631
Script:
#!/bin/bash # Corrected script to check for other occurrences of the old variable echo "Checking for other occurrences of --ti-lowcode-base-bg:" rg --type less "--ti-lowcode-base-bg" # Corrected script to check for occurrences of the new variable echo "Checking for occurrences of --te-common-bg-container:" rg --type less "--te-common-bg-container" # Check for any potential color contrast issues echo "Checking for potential color contrast issues:" rg --type less "color:|background:|background-color:"Length of output: 99631
Script:
#!/bin/bash # Corrected script to check for other occurrences of the old variable echo "Checking for other occurrences of --ti-lowcode-base-bg:" rg --type less -- '--ti-lowcode-base-bg' # Corrected script to check for occurrences of the new variable echo "Checking for occurrences of --te-common-bg-container:" rg --type less -- '--te-common-bg-container' # Check for any potential color contrast issues echo "Checking for potential color contrast issues:" rg --type less "color:|background:|background-color:"Length of output: 105505
packages/theme/light/materials.less (1)
21-21: Approved: Good move towards standardizationThe update of
--ti-lowcode-materials-block-transfer-header-bg-colorto usevar(--te-common-bg-container)is a positive change. This modification aligns well with the PR's objective of improving component styling and establishing a common theme. By using a more standardized background color variable, you're enhancing the consistency of the UI and potentially simplifying future maintenance of the theming system.packages/theme/dark/canvas.less (1)
27-27:⚠️ Potential issueFix typo in CSS custom property and standardize naming convention
There's a typo in the new value for
--ti-lowcode-canvas-menu-item-disabled-color. It currently has three dashes instead of two:- --ti-lowcode-canvas-menu-item-disabled-color: var(---te-common-text-weaken); + --ti-lowcode-canvas-menu-item-disabled-color: var(--te-common-text-weaken);Please correct this typo to ensure the property works as intended.
Additionally, this change reinforces the inconsistency in prefix naming noted in the previous comment (
ti-lowcode-*vste-common-*). Consider standardizing these prefixes across the codebase for better maintainability.Let's check if this typo exists elsewhere in the codebase:
#!/bin/bash # Description: Check for usage of ---te-common- (triple dash) in less files echo "Files with ---te-common- (triple dash):" rg --type less '---te-common-' -lIf this script returns any results, those occurrences should be fixed as well.
packages/plugins/datasource/src/DataSourceRemoteDataResult.vue (1)
108-108: Verify the border color change for consistency with the design system.The border color for
.tiny-buttonhas been updated fromvar(--ti-lowcode-button-default-border-color)tovar(--te-common-border-default). While this change likely aims to improve consistency across the application, it's important to ensure it aligns with the intended design.Please confirm that:
- This change is intentional and part of a broader update to the theming system.
- The new color variable
--te-common-border-defaultis defined and produces the expected visual result.- This change is consistent with other button styles across the application.
You may want to run the following script to check for consistency:
This will help ensure that the change is applied consistently across the codebase.
✅ Verification successful
Border color change verified and consistent with the design system.
The update from
ti-lowcode-button-default-border-colorto--te-common-border-defaulthas been successfully applied across the codebase. No instances of the old variable remain, and the new variable is used consistently in the relevant components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistency in button border color usage # Search for other occurrences of the old variable echo "Occurrences of the old variable:" rg --type vue --type less "ti-lowcode-button-default-border-color" # Search for occurrences of the new variable echo "Occurrences of the new variable:" rg --type vue --type less "te-common-border-default" # Search for other button border color definitions echo "Other button border color definitions:" rg --type vue --type less "border-color:.*button"Length of output: 492
Script:
#!/bin/bash # Description: Check for consistency in button border color usage # Search for other occurrences of the old variable echo "Occurrences of the old variable:" rg "ti-lowcode-button-default-border-color" -g '*.vue' -g '*.less' # Search for occurrences of the new variable echo "Occurrences of the new variable:" rg "te-common-border-default" -g '*.vue' -g '*.less' # Search for other button border color definitions echo "Other button border color definitions:" rg "border-color:.*button" -g '*.vue' -g '*.less'Length of output: 3884
packages/layout/src/DesignSettings.vue (3)
61-63: Improved active tab indicator stylingThe changes to the
.tiny-tabs__active-barclass enhance the visual feedback for the active tab. The increased height makes the indicator more noticeable, and the use of a CSS variable for the background color supports theming and maintains consistency across the application.
83-85: Refined tab title stylingThe addition of a 2px bottom padding to the
.tiny-tabs__item__titleclass is a subtle improvement that enhances the visual appearance of the tab titles. This minor adjustment contributes to the overall polish of the component's design.
88-90: Clarify the purpose of the new CSS ruleA new CSS rule has been added for
.tiny-tabs__nav-wrap-not-separator::afterwith a z-index of 2. The purpose of this rule is not immediately clear from the context provided.Could you please clarify the intended purpose of this new CSS rule? Is it related to styling a separator or creating a shadow effect? Understanding its function would help ensure it aligns with the overall design goals.
To help investigate this, let's check if there are any related styles or usage of this class in other files:
packages/theme/dark/metaComponent.less (1)
12-12: LGTM! Verify consistency with other components.The update to
--ti-lowcode-meta-codeEditor-border-colorlooks good. Usingvar(--te-common-border-default)aligns with the goal of implementing a common theme and should improve consistency across the application.To ensure this change maintains visual consistency, please run the following script to check for any other components still using the old border color variable:
If the script returns results, consider updating those components to use the new
--te-common-border-defaultvariable for consistency.✅ Verification successful
Verified: No usages of the old border color variable found.
The update to
--ti-lowcode-meta-codeEditor-border-colorhas been successfully verified. The old variable--ti-lowcode-button-default-border-coloris no longer used, ensuring consistency across the application.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for components still using the old border color variable # Search for the old variable name in all less files rg --type less "var\(--ti-lowcode-button-default-border-color\)" packages/Length of output: 76
Script:
#!/bin/bash # Description: Search for any definitions or usages of the old border color variable # Search for the exact variable definition rg --type less "--ti-lowcode-button-default-border-color" packages/ # Additionally, search for variations or partial matches of the variable name rg --type less "--ti-lowcode.*border.*color" packages/Length of output: 997
Script:
#!/bin/bash # Description: Search for any definitions or usages of the old border color variable using corrected patterns # Search for the exact variable definition rg --type less -e "--ti-lowcode-button-default-border-color" packages/ # Additionally, search for variations or partial matches of the variable name rg --type less -e "--ti-lowcode.*border.*color" packages/Length of output: 21131
packages/configurator/src/slot-configurator/SlotConfigurator.vue (2)
Line range hint
1-201: Summary of review
- The styling changes to the
.e__switch-coreclass have been addressed, with suggestions to ensure proper contrast and visual distinction.- Recommendations for improving accessibility and internationalization have been provided.
- The overall structure and logic of the component appear to be sound.
Please consider implementing the suggested improvements to enhance the component's usability and maintainability. If you have any questions or need further clarification on any of the points raised, feel free to ask.
174-178: 🛠️ Refactor suggestionConsider potential contrast issues with the updated styling.
The changes to the
.e__switch-coreclass align with the goal of enhancing visual consistency by using the--te-common-bg-containervariable for both border and background colors. This approach supports a more theme-centric styling strategy.However, using the same color for both border and background might lead to a lack of visual distinction between these elements. This could potentially impact the usability of the switch component.
Consider the following suggestions:
- Ensure that there's sufficient contrast between the switch and its container.
- If needed, use a slightly different shade for the border to maintain visual separation:
.e__switch-core { border: 1px solid var(--te-common-border-color, var(--te-common-bg-container)); background: var(--te-common-bg-container); }This approach allows for a separate border color while falling back to the background color if not specified.
To ensure the changes don't negatively impact the component's appearance, please run the following script to check for other occurrences of the old variable and potential inconsistencies:
packages/settings/styles/src/components/background/LinearGradient.vue (2)
91-94: LGTM: Improved angle input handling.The
inputAnglefunction has been refactored to be more concise and effective. It now correctly updates the state and triggers the rotation, ensuring consistent behavior across the component.
Line range hint
1-190: Overall, good improvements to the LinearGradient component.The changes made to this component enhance its functionality and readability. The refactoring of functions like
angleChangeandinputAngleimproves the code structure. The simplification of the color matching regex is also a positive change.To ensure the changes work as expected:
- Verify that the linear gradient updates correctly when changing the angle.
- Check if the gradient colors are applied correctly from
state.colorList.- Test edge cases for angle input (e.g., negative values, values > 360).
Run the following commands to check for any potential issues:
These checks will help ensure that the changes are consistent throughout the component and that no unintended side effects were introduced.
packages/configurator/src/js-slot-configurator/JsSlotConfigurator.vue (1)
237-241: Update to theme variables improves consistencyThe changes to the border and background properties align the component with a more generic theming system:
- Border:
var(--ti-lowcode-base-bg)→var(--te-common-bg-container)- Background:
var(--ti-lowcode-base-bg)→var(--te-common-bg-container)These updates likely improve the overall consistency of the application's theming. However, ensure that:
- The new
--te-common-bg-containervariable is defined and accessible in all necessary contexts.- The visual appearance of the switch component still meets the design requirements with these new variables.
To verify the usage and definition of the new CSS variable, run the following script:
packages/settings/styles/src/components/background/BackgroundImageSetting.vue (2)
177-178: Approved: Consistent use of standardized color variables.The changes to the
.row-content-itemclass align with the overall theme standardization. Using semantic color variables liketext-secondaryandbg-containerimproves code readability and maintains consistency across the application.
197-198: Approved with a suggestion: Review hover and selected state distinctions.The updates to hover and selected states continue the good practice of using standardized color variables. However, there's a potential usability concern:
- Both hover and selected states use
var(--te-common-bg-container)for the background color.- This might make it difficult for users to distinguish between hovering over an unselected item and a selected item.
Consider differentiating the background colors for hover and selected states to improve user experience. You might want to use a slightly different shade for the hover state, such as
var(--te-common-bg-hover)if available.To verify the impact of this change, please run the following script:
If the script doesn't find any results, consider creating a new variable for hover states or using a function to slightly modify the container background color for hover states.
Also applies to: 202-203
packages/common/component/MetaCodeEditor.vue (1)
297-297: LGTM: Color variable updated for consistency.The color variable for the
.empty-colorclass has been updated to use a more generic weakened text color. This change aligns with the overall theme updates mentioned in the PR objectives.To ensure the new color variable is defined and consistent across the project, run the following script:
✅ Verification successful
Verified: New color variable is consistently used across the codebase.
The usage of
var(--te-common-text-weaken)has been successfully updated and is consistently applied in multiple components and stylesheets. No remnants of the old color variable were found, ensuring consistency and alignment with the theme updates.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence and usage of the new color variable rg --type=less --type=vue "var\(--te-common-text-weaken\)"Length of output: 88
Script:
#!/bin/bash # Verify the existence and usage of the new color variable across all files rg "var\(--te-common-text-weaken\)"Length of output: 1649
packages/common/component/ConfigItem.vue (1)
603-603: Approve color update for property descriptionThe color for
.prop-descriptionhas been updated to use a more standardized variablevar(--te-common-text-weaken). This change likely improves consistency across the application's theme.To ensure the new color works well across all themes, please run the following verification:
This script will help verify that the new color variable is properly defined and used consistently across the project.
|
|
|
|
|
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/theme/base/src/page/base-config-page.less (2)
6-35: Comprehensive scrollbar styling with room for optimization.The new scrollbar styles are well-structured and use CSS variables for colors, which is great for maintainability and theming. The different sizes for standard and thin scrollbars provide flexibility for various UI contexts.
Consider replacing the universal selector (*) with more specific selectors to potentially improve performance, especially for large DOMs. For example:
body, .lowcode-scrollbar, .lowcode-scrollbar-thin { // existing styles }This change would limit the scope of the scrollbar styles to the body and specific classes, potentially reducing the number of elements these styles are applied to.
122-132: Useful utility classes added with room for improvement.The new utility classes are valuable additions:
.global-desc-infouses a CSS variable for text color, which is great for theming and consistency..text-ellipsis-multipleprovides a multi-line text truncation with ellipsis, which is a commonly needed feature.For
.text-ellipsis-multiple, consider adding a cross-browser fallback for the multi-line truncation. The current implementation uses webkit-specific properties, which may not work in all browsers. Here's a more cross-browser compatible version:.text-ellipsis-multiple { overflow: hidden; display: -webkit-box; -webkit-box-orient: vertical; -webkit-line-clamp: 2; /* number of lines to show */ line-clamp: 2; max-height: 3em; /* fallback */ }This version adds
line-clampfor future compatibility and amax-heightas a fallback for browsers that don't supportline-clamp.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- packages/theme/base/src/base.less (1 hunks)
- packages/theme/base/src/dark/dark-common.js (1 hunks)
- packages/theme/base/src/index.js (1 hunks)
- packages/theme/base/src/light/light-common.js (1 hunks)
- packages/theme/base/src/page/base-config-page.less (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/theme/base/src/base.less
- packages/theme/base/src/dark/dark-common.js
- packages/theme/base/src/index.js
- packages/theme/base/src/light/light-common.js
🔇 Additional comments (3)
packages/theme/base/src/page/base-config-page.less (3)
49-51: LGTM!The simplified body styles improve maintainability.
95-96: Improved SVG icon styling.The use of
currentColorfor the SVG icon fill is an excellent choice. This approach allows the icon color to automatically inherit from its parent element, providing maximum flexibility without the need for additional CSS variables.This implementation is actually more versatile than using a specific CSS variable for the icon color, as it adapts to any color context it's placed in.
104-104: Consistent use of CSS variables for non-webkit browsers.The update to use a CSS variable for the scrollbar color in non-webkit browsers is a good improvement. This change ensures consistency with the webkit-specific styles and enhances the overall maintainability of the theme.
chilingling
added
ready_for_review
* feat(theme): component style && common theme * feat(theme): merge refactor/develop * feat(theme): common theme & fix review * feat(theme): common theme & fix review * feat(theme): common component review * feat(theme): check color and fix * feat(style): modify style panel widget margin && style * fix(style): fix style layout no working * fix(style): fix review * fix(style): fix review * fix(style): fix review * fix(style): fix review * fix(style): fix review * fix(style): fix review
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Removals
Updates