Skip to content
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(refactor/theme): change incorrect css variables to the common variables and delete theme/base.less #1049

Merged
merged 12 commits into from
Jan 21, 2025

Conversation

wenmine
Copy link
Collaborator

@wenmine wenmine commented Jan 16, 2025

English | 简体中文

PR

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our Commit Message Guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • Built its own designer, fully self-validated

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

Background and solution

此pr做的事情:

  1. 将文件里的变量滥用调整先调整为common变量
  2. 收敛和调整模块变量
  • 将模块less文件里的右侧值的部分,不再使用ti-lowcode-xxx,不再依赖其他的模块变量,只依赖common和base,保持模块变量的干净纯粹
  • 根据当前的场景和当前的值,去common里拿对应场景的common变量
  • 将里面写死颜色的地方,替换成common变量
    image
  1. 将 对 light/base.less 和dark/base.less 变量的使用换成theme/base.less和common.less,不再单独分light和dark的base。替换之后,将light/base.less 和dark/base.less 文件删除
  2. 整理部分的light/variable.less和dark/variable.less

整体目的

  1. 消灭ti-lowcode的引用
  2. 消灭一个非通用变量被滥用的情况
  3. 亮色和暗色场景使用的变量一致,需要审视不一致的情况

剩余要做的事情:

  • 0. 重要优先 将标识为:【暗色和亮色不一致】【需要确认】【暗色独有】【需要注意】【需注意】【暗色和亮色存在差异】的遗留事项,跟设计师进行确认
  • gpt变量文件,由于存在特殊性(亮色和暗色主题颜色一致),现先采用写死色值解决,后续针对此类问题待确认方案
  1. 整理light和dark里面的剩余的模块变量,具体做法为:
    举例:
    image
    • 将模块less文件里的右侧值的部分,不再使用ti-lowcode-xxx,不再依赖其他的模块变量,只依赖common和base,保持模块变量的干净纯粹
    • 根据当前的场景和当前的值,去common里拿对应场景的common变量
    • 将里面写死颜色的地方,替换成common变量
    • 由于历史色值比较混乱的原因,所以可能会出现common变量和base变量里面没有的色值,这个时候,可以根据场景语义化,与设计师对齐,选择一个匹配的common变量进行修正。
    • 正常来说,dark和light的场景变量名应该的一致的,应该引用同一个common场景变量;后期要审视不一致的场景,与设计师对齐。看看是否可以调节成同一个场景的common变量。
  2. 整理 light/variable.less和dark/variable.less,目的是删除这两个文件,具体做法:
  3. 挨个查询当前在variable.less里的变量在哪里被使用,审视使用合理性,审视的做法是:
    • 若是没有地方用到,直接删除
    • 若是被模块变量文件用到了,则在模块文件里直接使用common变量即可
    • 若是被vue文件直接使用了,有两种修改方式:
      1.直接改成common变量,等待之后调整
      2.一步到位,看是哪一个模块的,然后新增模块变量,模块变量的值是common变量,然后vue文件使用的是模块变量。
  4. 最后,当variable.less和dark/base light/base.less都清理完之后,将模块变量回归到模块中。
  5. 模块变量回归模块后,再排查一遍该模块有没有直接使用--te-common的,有的话,需要转成模块变量。去保证整个链路是这样的: base/common less -> module less -> vue文件
  6. 将模块的--ti-lowcode 改名为--te-xxx 具体取名内部商量
  7. 需要单独审视,每个包与主题的关系和依赖,是否存在不依赖主题的情况,如:@opentiny/tiny-engine-setting-design,这种需要单独设置自己的变量。这块需要拉上chilling和qihex评审

注意点:需要看一下暗色主题
注意:对于一些分割线,使用的变量都是--te-common-border-divider,不要混用 --te-common-border-default了。可能需要全量排查一遍,可以排查分别设置 border-top,border-left, border-right,border-bottom的地方,看看有没有类似的分割线场景

image

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Summary by CodeRabbit

Summary by CodeRabbit

Based on the comprehensive review of the provided changes, here's a concise summary of the release notes:

  • Style Updates

    • Comprehensive redesign of color scheme across multiple components.
    • Replaced legacy color variables with new design system tokens.
    • Updated background, text, border, and icon colors for improved visual consistency.
  • Design System Migration

    • Transitioned from --ti-lowcode-* to --te-common-* color variables.
    • Enhanced visual presentation across toolbars, plugins, and components.
    • Maintained existing functionality while updating styling.
  • User Interface Refinements

    • Subtle improvements to hover states, text colors, and component backgrounds.
    • Consistent styling across different sections of the application.

No functional changes were introduced, and the core logic of components remains unchanged.

Copy link
Contributor

coderabbitai bot commented Jan 16, 2025

Walkthrough

This pull request involves a comprehensive update to the theming system across multiple files in the packages/theme/dark and packages/theme/light directories. The changes primarily focus on replacing existing CSS custom properties (variables) with a new set of variables, transitioning from a --ti-lowcode-* naming convention to a --te-common-* naming convention. This refactoring aims to standardize and simplify the color management across the application's light and dark themes, along with specific styling updates in various Vue components.

Changes

File Change Summary
packages/canvas/container/src/components/CanvasDivider.vue Updated background and text color variables
packages/common/component/PluginBlockList.vue Modified border and hover color variables
packages/plugins/page/src/PageGeneral.vue Updated input group and tip element color variables
packages/plugins/page/src/PageHome.vue Changed tip text color variables
packages/plugins/page/src/PageInputOutput.vue Modified input-output component text color
packages/plugins/schema/src/Main.vue Updated error indicator background color
packages/plugins/script/src/Main.vue Changed save button error indicator color
packages/settings/design/src/App.vue Removed a border color variable
packages/settings/design/src/components/PropertyCanvas.vue Updated collapse item header border color
packages/settings/design/src/components/PropertyList.vue Modified property list item border color
Multiple theme files Comprehensive replacement of color variables

Sequence Diagram

sequenceDiagram
    participant OldTheme as Old Theme Variables
    participant ThemeRefactor as Theme Refactoring
    participant NewTheme as New Theme Variables

    OldTheme->>ThemeRefactor: Identify existing variables
    ThemeRefactor->>ThemeRefactor: Map old variables to new naming convention
    ThemeRefactor->>NewTheme: Replace with standardized variables
    NewTheme-->>ThemeRefactor: Confirm variable replacement
Loading

Possibly related PRs

Suggested labels

refactoring, enhancement, ready_for_review

Suggested reviewers

  • chilingling
  • hexqi

Poem

🐰 Hop, hop, through the code we go,
Variables dancing, a colorful show!
From ti-lowcode to te-common we leap,
Refactoring themes, our promise we keep!
A rainbow of colors, now neat and bright,
Coding magic done with rabbit delight! 🌈✨


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added the refactor-main refactor/develop branch feature label Jan 16, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 11

🧹 Nitpick comments (10)
packages/theme/dark/tree.less (1)

2-3: Remove misplaced light theme comment

The comment "亮色主题特有" (light theme specific) appears in the dark theme file. This comment should be moved to the light theme file.

  --ti-lowcode-tree-icon-hover-color: var(--te-common-icon-hover);
- --ti-lowcode-tree-border-color: transparent; // 亮色主题特有
+ --ti-lowcode-tree-border-color: transparent;
packages/theme/dark/bridge.less (1)

3-5: Fix typo in variable name

The variable mappings use appropriate semantic variables, but there's a typo in the variable name: birdge should be bridge.

  --ti-lowcode-bridge-list-color: var(--te-common-text-primary);
  --ti-lowcode-bridge-list-bg: var(--te-common-bg-container);
- --ti-lowcode-birdge-editor-border-color: var(--te-common-border-default);
+ --ti-lowcode-bridge-editor-border-color: var(--te-common-border-default);

The semantic variable mappings are correct and appropriate for their usage.

packages/theme/light/bridge.less (1)

3-5: Fix typo in variable name: "birdge" → "bridge"

The variable --ti-lowcode-birdge-editor-border-color contains a typo. This should be fixed to maintain consistency and prevent future confusion.

  --ti-lowcode-bridge-list-color: var(--te-common-text-primary);
  --ti-lowcode-bridge-list-bg: var(--te-common-bg-container);
- --ti-lowcode-birdge-editor-border-color: var(--te-common-border-default);
+ --ti-lowcode-bridge-editor-border-color: var(--te-common-border-default);

The semantic mapping of variables looks good - using appropriate common variables for text, background, and border colors.

packages/theme/dark/life-cycles.less (1)

2-6: Consider translating Chinese comment to English

For better international collaboration, consider translating the Chinese comment "生命周期" to English "Life Cycle".

- // 生命周期
+ // Life Cycle

The variable mappings look good and are semantically appropriate:

  • Using --te-common-text-disabled for alert and disabled states
  • Using --te-common-bg-container for hover state
  • Using --te-common-border-default for editor border
packages/theme/light/life-cycles.less (1)

2-6: Consider translating Chinese comment to English

For better international collaboration, consider translating the Chinese comment "生命周期" to English "Life Cycle".

- // 生命周期
+ // Life Cycle

The implementation perfectly mirrors the dark theme, ensuring consistent variable usage across themes.

packages/theme/dark/metaComponent.less (1)

3-35: Consider theme-specific variable mappings.

While using common variables improves maintainability, mapping identical --te-common-* values in both light and dark themes might reduce theme distinctiveness. Consider using theme-specific variants of common variables where appropriate.

packages/theme/light/block.less (1)

3-28: Consider translating Chinese comments to English.

For better international collaboration and maintenance, consider translating the Chinese comments to English.

packages/theme/dark/gpt-dialog.less (1)

42-45: Consider documenting the gradient color values.

The gradient colors create a visually appealing background, but their purpose and usage could be better documented.

Add comments explaining the gradient's purpose:

  // chatGPT背景渐变色
-  --ti-lowcode-chat-bg-top-color: rgb(227, 236, 255);
-  --ti-lowcode-chat-bg-mid-color: rgb(236, 245, 255);
-  --ti-lowcode-chat-bg-bottom-color: rgb(255, 255, 255);
+  // Creates a subtle fade from blue to white for depth
+  --ti-lowcode-chat-bg-top-color: rgb(227, 236, 255);    /* Top: Light blue */
+  --ti-lowcode-chat-bg-mid-color: rgb(236, 245, 255);    /* Middle: Lighter blue */
+  --ti-lowcode-chat-bg-bottom-color: rgb(255, 255, 255); /* Bottom: White */
packages/theme/light/variable.less (1)

Line range hint 1-220: Consider translating Chinese comments to English for better international collaboration.

The file contains comments in Chinese. For better maintainability and international collaboration, consider translating them to English.

Example translations for the first few comments:

- // 亮色主题
+ // Light theme
- // 基础配置,前缀为 --base
+ // Base configuration, prefix: --base
- // ------ 主要的公共主题色,前缀为 --ti-lowcode-common -------
+ // ------ Main common theme colors, prefix: --ti-lowcode-common -------
packages/theme/dark/variable.less (1)

35-38: Enhance visual feedback for SVG button states.

Using the same color (--te-common-text-primary) for all button states (normal, hover, active) reduces visual feedback. Consider using different semantic colors for each state.

-  --ti-lowcode-component-svg-button-color: var(--te-common-text-primary);
-  --ti-lowcode-component-svg-button-hover-color: var(--te-common-text-primary);
-  --ti-lowcode-component-svg-button-active-color: var(--te-common-text-primary);
+  --ti-lowcode-component-svg-button-color: var(--te-common-text-secondary);
+  --ti-lowcode-component-svg-button-hover-color: var(--te-common-text-primary);
+  --ti-lowcode-component-svg-button-active-color: var(--te-common-text-checked);
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ed569f6 and a839fb4.

📒 Files selected for processing (44)
  • packages/canvas/container/src/components/CanvasDivider.vue (2 hunks)
  • packages/common/component/PluginBlockList.vue (2 hunks)
  • packages/plugins/page/src/PageGeneral.vue (2 hunks)
  • packages/plugins/page/src/PageHome.vue (1 hunks)
  • packages/plugins/page/src/PageInputOutput.vue (1 hunks)
  • packages/plugins/schema/src/Main.vue (1 hunks)
  • packages/plugins/script/src/Main.vue (1 hunks)
  • packages/settings/design/src/App.vue (0 hunks)
  • packages/settings/design/src/components/PropertyCanvas.vue (1 hunks)
  • packages/settings/design/src/components/PropertyList.vue (1 hunks)
  • packages/theme/dark/base.less (0 hunks)
  • packages/theme/dark/block.less (1 hunks)
  • packages/theme/dark/bridge.less (1 hunks)
  • packages/theme/dark/events.less (1 hunks)
  • packages/theme/dark/gpt-dialog.less (1 hunks)
  • packages/theme/dark/help.less (1 hunks)
  • packages/theme/dark/i18n.less (1 hunks)
  • packages/theme/dark/index.less (0 hunks)
  • packages/theme/dark/life-cycles.less (1 hunks)
  • packages/theme/dark/materials.less (1 hunks)
  • packages/theme/dark/metaComponent.less (1 hunks)
  • packages/theme/dark/pageManage.less (1 hunks)
  • packages/theme/dark/plugin-js.less (1 hunks)
  • packages/theme/dark/toolbar.less (1 hunks)
  • packages/theme/dark/tree.less (1 hunks)
  • packages/theme/dark/tutorial.less (1 hunks)
  • packages/theme/dark/variable.less (7 hunks)
  • packages/theme/light/base.less (0 hunks)
  • packages/theme/light/block.less (1 hunks)
  • packages/theme/light/bridge.less (1 hunks)
  • packages/theme/light/events.less (1 hunks)
  • packages/theme/light/gpt-dialog.less (1 hunks)
  • packages/theme/light/help.less (1 hunks)
  • packages/theme/light/i18n.less (1 hunks)
  • packages/theme/light/index.less (0 hunks)
  • packages/theme/light/life-cycles.less (1 hunks)
  • packages/theme/light/materials.less (1 hunks)
  • packages/theme/light/metaComponent.less (1 hunks)
  • packages/theme/light/pageManage.less (1 hunks)
  • packages/theme/light/plugin-js.less (1 hunks)
  • packages/theme/light/toolbar.less (1 hunks)
  • packages/theme/light/tree.less (1 hunks)
  • packages/theme/light/tutorial.less (1 hunks)
  • packages/theme/light/variable.less (6 hunks)
💤 Files with no reviewable changes (5)
  • packages/theme/dark/index.less
  • packages/theme/light/index.less
  • packages/settings/design/src/App.vue
  • packages/theme/light/base.less
  • packages/theme/dark/base.less
✅ Files skipped from review due to trivial changes (8)
  • packages/plugins/page/src/PageHome.vue
  • packages/plugins/schema/src/Main.vue
  • packages/plugins/page/src/PageInputOutput.vue
  • packages/settings/design/src/components/PropertyCanvas.vue
  • packages/common/component/PluginBlockList.vue
  • packages/plugins/script/src/Main.vue
  • packages/settings/design/src/components/PropertyList.vue
  • packages/plugins/page/src/PageGeneral.vue
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: push-check
🔇 Additional comments (22)
packages/theme/light/tree.less (1)

2-3: Verify intentional theme difference

The border color differs between themes:

  • Dark theme: transparent
  • Light theme: var(--te-common-border-default)

This appears intentional but should be verified to ensure it provides the desired visual effect in both themes.

packages/theme/dark/plugin-js.less (1)

3-4: LGTM! Semantic variable mapping is correct

The variable mappings use appropriate semantic variables:

  • Background uses --te-common-bg-default
  • Help link uses --te-common-text-link
packages/theme/light/plugin-js.less (1)

3-4: LGTM! Consistent with dark theme

The variable mappings are identical to the dark theme, ensuring consistent behavior while allowing theme-specific colors through the semantic variables.

packages/theme/dark/tutorial.less (1)

2-3: LGTM! Consistent with light theme

The variable mappings are semantically correct and maintain consistency with the light theme implementation:

  • Using --te-common-text-secondary for primary text
  • Using --te-common-bg-default for primary background
packages/theme/light/tutorial.less (1)

2-3: LGTM! Maintains theme consistency

The changes mirror the dark theme implementation perfectly, ensuring consistent variable usage across themes.

packages/theme/dark/i18n.less (1)

3-9: LGTM! Verify visual consistency after variable updates.

The variable mappings look correct and follow the new theming system. The changes maintain semantic meaning while standardizing to common variables.

Please verify that the visual appearance remains consistent after switching from specific colors to common theme variables, especially for:

  • Loading text color (now using --te-common-text-weaken)
  • Loading SVG color (now using --te-common-text-primary)
packages/theme/dark/pageManage.less (1)

2-7: LGTM! Consistent use of semantic color variables.

The variable mappings follow a clear pattern:

  • Primary text for titles and headings
  • Secondary text for general content
  • Weaken text for less prominent elements
  • Default background for containers
packages/theme/light/help.less (2)

1-7: LGTM! Well-scoped component variables.

Good practice scoping the guide variables to specific components rather than placing them in :root.


8-12: Verify corresponding dark theme variables exist.

Please ensure these help box variables are also defined in packages/theme/dark/help.less with appropriate dark theme mappings.

✅ Verification successful

All help box variables are properly defined in dark theme

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if dark theme has corresponding help variables
rg --type less "help-plugin-box" packages/theme/dark/

Length of output: 102


Script:

#!/bin/bash
# View content of dark theme help.less
cat packages/theme/dark/help.less

Length of output: 705

packages/theme/dark/toolbar.less (1)

3-11: LGTM! Variables are consistently mapped to common theme variables.

The changes align well with the standardization effort, replacing specific toolbar variables with common theme variables.

Let's verify the consistency between dark and light themes:

✅ Verification successful

✅ Verified: Dark and light themes are perfectly synchronized

Both themes define identical toolbar variables with consistent mappings to common theme variables, ensuring a uniform theming system.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Compare toolbar variable definitions between themes
diff <(rg --no-filename "^[[:space:]]*--ti-lowcode-toolbar-.*:" packages/theme/dark/toolbar.less | sort) <(rg --no-filename "^[[:space:]]*--ti-lowcode-toolbar-.*:" packages/theme/light/toolbar.less | sort)

Length of output: 319


Script:

#!/bin/bash
# List toolbar variables in dark theme
echo "Dark theme toolbar variables:"
cat packages/theme/dark/toolbar.less | grep -E "^[[:space:]]*--ti-lowcode-toolbar-.*:" | sort

echo -e "\nLight theme toolbar variables:"
# List toolbar variables in light theme
cat packages/theme/light/toolbar.less | grep -E "^[[:space:]]*--ti-lowcode-toolbar-.*:" | sort

Length of output: 1622

packages/theme/dark/help.less (2)

3-6: LGTM! Guide overlay variables are properly mapped.

The text and background colors for the guide overlay are consistently mapped to common theme variables.


9-12: LGTM! Help box variables are properly mapped with new hover state.

The addition of --ti-lowcode-help-box-item-hover-bg-color improves hover state consistency.

packages/theme/light/materials.less (1)

3-21: LGTM! Materials variables are well-documented and properly mapped.

The changes maintain clear semantic meaning while transitioning to common theme variables.

Let's verify the consistency between light and dark materials:

✅ Verification successful

Theme-specific variations are intentionally implemented

The differences between light and dark materials are well-documented and semantically appropriate:

  • Dark theme uses transparent borders for icon buttons
  • Background and text colors are properly adjusted for dark mode visibility
  • Comments in dark theme explicitly mark intentional differences
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Compare materials variable definitions between themes
diff <(rg --no-filename "^[[:space:]]*--ti-lowcode-materials-.*:" packages/theme/dark/materials.less | sort) <(rg --no-filename "^[[:space:]]*--ti-lowcode-materials-.*:" packages/theme/light/materials.less | sort)

Length of output: 327


Script:

#!/bin/bash
# Check if both files exist and show their contents
echo "=== Checking dark/materials.less ==="
cat packages/theme/dark/materials.less
echo -e "\n=== Checking light/materials.less ==="
cat packages/theme/light/materials.less

Length of output: 2355

packages/theme/light/metaComponent.less (1)

3-35: Verify the impact of theme variable changes on component styling.

The transition from --ti-lowcode-* to --te-common-* variables looks good. However, we should verify that these changes maintain consistent styling between light and dark themes.

Let's verify the usage of these variables across components:

✅ Verification successful

Theme variable changes are properly implemented and consistent

The transition from --ti-lowcode-* to --te-common-* variables maintains consistent styling across light and dark themes, with proper variable scoping and references in all dependent components.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find components using the old variable names to ensure all occurrences are updated

# Search for any remaining usage of old variable names
rg --type-add 'style:*.{less,css,vue}' -t style 'var\(--ti-lowcode-(meta|component)-'

# Search for components using the new variables to verify correct implementation
rg --type-add 'style:*.{less,css,vue}' -t style 'var\(--te-common-(text|bg|border)-'

Length of output: 120115

packages/theme/dark/block.less (1)

12-23: Address noted theme inconsistencies.

The comments indicate several inconsistencies between light and dark themes:

  1. --ti-lowcode-component-block-version-list-current-version-color is dark theme-specific
  2. --ti-lowcode-component-block-list-setting-btn-color has different values in light and dark themes

Consider standardizing these differences or documenting the rationale for maintaining them.

packages/theme/dark/events.less (1)

Line range hint 2-38: LGTM! The variable changes align with the standardization effort.

The changes successfully transition from custom --ti-lowcode-* variables to common --te-common-* variables, maintaining semantic meaning and visual consistency.

packages/theme/light/events.less (1)

2-15: LGTM! The variable changes maintain consistency with the dark theme.

The changes successfully mirror the dark theme's transition to common variables, ensuring visual consistency across themes.

Also applies to: 17-38

packages/theme/dark/gpt-dialog.less (1)

1-41: LGTM! The color scheme is well-defined and consistent.

The changes establish a clear color hierarchy for the dialog interface, with appropriate contrast ratios for text readability.

packages/canvas/container/src/components/CanvasDivider.vue (1)

204-204: LGTM! The styling changes align with the new theming system.

The component successfully transitions to using common variables while maintaining its visual functionality.

Also applies to: 226-226

packages/theme/light/variable.less (1)

Line range hint 9-217: LGTM! The variable standardization is comprehensive and well-structured.

The changes successfully transition to using common variables while maintaining semantic meaning and visual consistency.

packages/theme/dark/variable.less (2)

Line range hint 1-238: Overall, good progress on variable standardization.

The changes successfully transition most variables to the new --te-common-* system, improving consistency. However, please address:

  1. The circular reference in the error color variable
  2. Verify the semantic meaning of certain variables
  3. Consider enhancing visual feedback for interactive elements
  4. Replace remaining hard-coded values with semantic variables

22-23: Verify semantic meaning of grid light color.

Using --te-common-bg-default for --ti-grid-light-color in a dark theme seems counterintuitive. Please verify if this is the intended behavior or if it should use a different semantic variable.

✅ Verification successful

Variable usage is semantically correct

The "light" in --ti-grid-light-color refers to relative lightness compared to surrounding elements, not the theme type. Using --te-common-bg-default in dark theme provides appropriate contrast as it represents a lighter shade (#212329) compared to darker surrounding elements.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other usages of grid light color to understand its context
rg --type less "ti-grid-light-color" -B 2 -A 2

Length of output: 348


Script:

#!/bin/bash
# Search for grid light color in all files
rg "ti-grid-light-color"

# Search for grid styles in light theme
fd "variable.less" packages/theme/light

# Look for grid component styles
fd "grid" packages/theme -e less

# Search for any grid documentation
fd "grid" -e md

Length of output: 2275


Script:

#!/bin/bash
# Check light theme variable definition
cat packages/theme/light/variable.less

# Search for te-common-bg-default definition and usage
rg "te-common-bg-default" -B 2 -A 2

# Search for ti-base-color-light
rg "ti-base-color-light" -B 2 -A 2

Length of output: 104749

packages/theme/light/i18n.less Outdated Show resolved Hide resolved
packages/theme/light/pageManage.less Outdated Show resolved Hide resolved
packages/theme/light/toolbar.less Outdated Show resolved Hide resolved
packages/theme/dark/materials.less Show resolved Hide resolved
packages/theme/light/block.less Show resolved Hide resolved
packages/theme/light/events.less Outdated Show resolved Hide resolved
packages/theme/light/variable.less Show resolved Hide resolved
packages/theme/dark/variable.less Outdated Show resolved Hide resolved
packages/theme/dark/variable.less Outdated Show resolved Hide resolved
packages/theme/dark/variable.less Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (1)
packages/theme/dark/variable.less (1)

25-25: ⚠️ Potential issue

Fix circular reference in error color variable.

The error color variable appears to be self-referential through --te-common-color-error.

🧹 Nitpick comments (6)
packages/theme/dark/variable.less (1)

219-229: Inconsistent usage of base vs. common variables.

The code mixes usage of --te-base-* and --te-common-* variables:

  • Line 219: Uses --te-base-green-40
  • Line 221: Uses --te-common-text-checked

Consider standardizing to use common semantic variables instead of base color variables for better maintainability. For example:

- --ti-lowcode-description-color: var(--te-base-green-40);
+ --ti-lowcode-description-color: var(--te-common-text-success);
packages/theme/light/variable.less (2)

13-17: Consider differentiating button states for better visual feedback.

All SVG button states (normal, hover, active) currently use the same text color --te-common-text-primary. Consider using different shades for hover and active states to provide better visual feedback to users.

  --ti-lowcode-component-svg-button-color: var(--te-common-text-primary);
- --ti-lowcode-component-svg-button-hover-color: var(--te-common-text-primary);
+ --ti-lowcode-component-svg-button-hover-color: var(--te-common-text-checked);
- --ti-lowcode-component-svg-button-active-color: var(--te-common-text-primary);
+ --ti-lowcode-component-svg-button-active-color: var(--te-common-text-checked);

143-144: Adjust menu hover states for better visual hierarchy.

The current hover states might create excessive contrast by using checked background and inverse text color. Consider using more subtle hover states.

- --ti-lowcode-main-menu-item-hover-bg: var(--te-common-bg-primary-checked);
- --ti-lowcode-main-menu-item-hover-color: var(--te-common-text-dark-inverse);
+ --ti-lowcode-main-menu-item-hover-bg: var(--te-common-bg-hover);
+ --ti-lowcode-main-menu-item-hover-color: var(--te-common-text-primary);
packages/theme/light/block.less (3)

3-6: Consider translating Chinese comments to English.

The variable mappings look good and align with the standardization objectives. However, for better maintainability and international collaboration, consider translating the Chinese comments to English.

-  --ti-lowcode-component-block-list-item-color: var(--te-common-text-secondary); // 区块列表颜色
-  --ti-lowcode-component-block-list-shortcut-title-color: var(--te-common-text-primary); // 快照标题颜色
-  --ti-lowcode-component-block-list-shortcut-bg: var(--te-common-bg-default); // 快照标题颜色
-  --ti-lowcode-component-block-list-item-active-bg: var(--te-common-bg-container); // 区块 active 背景色
+  --ti-lowcode-component-block-list-item-color: var(--te-common-text-secondary); // Block list color
+  --ti-lowcode-component-block-list-shortcut-title-color: var(--te-common-text-primary); // Shortcut title color
+  --ti-lowcode-component-block-list-shortcut-bg: var(--te-common-bg-default); // Shortcut background color
+  --ti-lowcode-component-block-list-item-active-bg: var(--te-common-bg-container); // Block active background color

19-19: Consider using a common variable for border-radius.

Instead of hardcoding the border-radius value, consider using a common variable for consistency across the theme.

-  --ti-lowcode-component-block-list-add-group-btn-border-radius: 6px;
+  --ti-lowcode-component-block-list-add-group-btn-border-radius: var(--te-common-border-radius-medium);

24-27: Consider differentiating hover states for better user experience.

Currently, the tag has identical colors for both normal and hover states, which might not provide enough visual feedback to users.

  --ti-lowcode-block-config-tag-color: var(--te-common-text-primary);
  --ti-lowcode-block-config-tag-bg: var(--te-common-bg-disabled);
-  --ti-lowcode-block-config-tag-hover-color: var(--te-common-text-primary);
-  --ti-lowcode-block-config-tag-hover-bg: var(--te-common-bg-disabled);
+  --ti-lowcode-block-config-tag-hover-color: var(--te-common-text-hover);
+  --ti-lowcode-block-config-tag-hover-bg: var(--te-common-bg-hover);
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a839fb4 and dc4ce93.

📒 Files selected for processing (10)
  • packages/settings/styles/src/components/background/ImageSetting.vue (0 hunks)
  • packages/settings/styles/src/components/background/RadialGradient.vue (0 hunks)
  • packages/theme/dark/materials.less (1 hunks)
  • packages/theme/dark/variable.less (6 hunks)
  • packages/theme/light/block.less (1 hunks)
  • packages/theme/light/events.less (1 hunks)
  • packages/theme/light/i18n.less (1 hunks)
  • packages/theme/light/pageManage.less (1 hunks)
  • packages/theme/light/toolbar.less (1 hunks)
  • packages/theme/light/variable.less (6 hunks)
💤 Files with no reviewable changes (2)
  • packages/settings/styles/src/components/background/ImageSetting.vue
  • packages/settings/styles/src/components/background/RadialGradient.vue
🚧 Files skipped from review as they are similar to previous changes (4)
  • packages/theme/light/events.less
  • packages/theme/light/i18n.less
  • packages/theme/dark/materials.less
  • packages/theme/light/toolbar.less
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: push-check
🔇 Additional comments (11)
packages/theme/light/pageManage.less (1)

2-7: Add missing tree node label variable.

The tree node label color variable is still missing. This issue was previously identified in an earlier review.

Add the following variable:

 :root {
   --ti-lowcode-page-manage-title-background-text-color: var(--te-common-text-primary);
   --ti-lowcode-page-manage-tip-text-color: var(--te-common-text-weaken);
   --ti-lowcode-page-manage-tree-text-background-color: var(--te-common-bg-default);
   --ti-lowcode-page-manage-tree-node-background-color: var(--te-common-bg-default);
   --ti-lowcode-page-manage-text-color: var(--te-common-text-secondary);
   --ti-lowcode-page-manage-input-head-text-color: var(--te-common-text-primary);
+  --ti-lowcode-page-manage-tree-node-label-color: var(--te-common-text-primary);
 }
packages/theme/dark/variable.less (5)

19-20: LGTM! Grid component variables updated correctly.

The grid component variables now properly reference the common theme variables for consistent styling.


38-52: LGTM! Component variables follow consistent pattern.

The variables for tabs, form errors, and plugin panel properly use semantic color variables from the common theme.


214-214: Review hover background opacity in dark theme.

The comment "需要确认" (needs confirmation) suggests uncertainty about the opacity value. Please verify if 0.1 opacity provides sufficient contrast in dark theme for hover states.


234-237: LGTM! Text color variables properly updated.

The numeric unit and block video tip color variables now correctly use semantic color variables.


32-35: Verify contrast ratio for SVG button states.

While the color transitions look correct, ensure that using the same color (--te-common-text-primary) for both normal and hover states provides sufficient visual feedback to users.

✅ Verification successful

SVG button states provide adequate visual feedback

The implementation correctly uses background color changes (--te-common-bg-prompt) to indicate hover and active states, while maintaining consistent text color for readability. This pattern is properly implemented across both light and dark themes.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other SVG button implementations to verify color contrast patterns
rg --type less "svg-button.*color:" -B 2 -A 2

Length of output: 1713

packages/theme/light/variable.less (4)

21-25: LGTM! Well-structured semantic variables.

The plugin panel variables are well-organized and properly use semantic variables for backgrounds and borders.


197-197: The hover background value needs adjustment for theme consistency.

The hardcoded rgba(255, 255, 255, 0.1) value with the "needs confirmation" comment still needs to be addressed.


213-213: LGTM! Proper use of semantic border variable.

The Monaco editor border color correctly uses the semantic variable --te-common-border-hover.


216-216: LGTM! Proper use of semantic text color.

The numeric unit text color correctly uses the semantic variable --te-common-text-primary.

packages/theme/light/block.less (1)

10-10: Remove duplicate variable declaration.

The variable --ti-lowcode-component-block-version-list-time-color is declared twice with the same value.

packages/theme/light/pageManage.less Show resolved Hide resolved
packages/theme/light/variable.less Show resolved Hide resolved
packages/theme/light/block.less Show resolved Hide resolved
@hexqi hexqi merged commit ceb8bae into opentiny:refactor/develop Jan 21, 2025
3 checks passed
@coderabbitai coderabbitai bot mentioned this pull request Jan 26, 2025
14 tasks
@coderabbitai coderabbitai bot mentioned this pull request Feb 5, 2025
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor-main refactor/develop branch feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants