Refactor theme tools for dynamic switch theme #2352
Conversation
WalkthroughThis pull request involves the deletion of multiple Gulp build scripts and related files within the project. The removed files included scripts for building low-code bundles, renderless components, themes, and Vue components, along with associated configuration files and LESS variable definitions. The changes also included updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (21)
💤 Files with no reviewable changes (20)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
shenjunjian
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (7)
packages/theme/src/button/old-theme.less (3)
2-2: LGTM: Button prefix class variable.The '@button-prefix-cls' variable is well-named and uses string interpolation for the CSS prefix. This approach promotes consistency and maintainability.
Consider adding a comment explaining the purpose of this variable for better code documentation.
4-7: LGTM: Button class and custom property definition.The CSS class is correctly defined using the prefix variable, and the custom property for font weight is well-structured.
Consider the following improvements:
- Translate the Chinese comment to English for better international collaboration.
- Consider using kebab-case for the custom property name to match CSS conventions, e.g., '--tv-button-font-weight'.
Here's a suggested revision:
.@{button-prefix-cls} { // Default button font weight --tv-button-font-weight: var(--tv-font-weight-thin); }
1-7: Overall: Well-structured theme file with minor improvement opportunities.This new 'old-theme.less' file for buttons is concise and follows good practices for LESS and CSS. It properly imports a custom stylesheet, defines a prefix variable for consistent class naming, and uses CSS custom properties for theming.
To further improve the file:
- Consider adding a brief file-level comment explaining the purpose of this 'old-theme' file and its relationship to other theme files.
- Ensure that the 'custom.less' import contains all necessary variables (like
@css-prefixand--tv-font-weight-thin) used in this file.- If this file is part of a larger theming system, consider documenting how it fits into the overall theme architecture of the project.
packages/theme/package.json (1)
Line range hint
3-35: Summary of changes and potential impactThe changes in this file contribute to the overall refactoring of the theme tools:
- Addition of
"type": "module"modernizes the package.- Simplification of the build script removes potentially unnecessary steps.
- Minor reorganization of scripts improves file structure.
These changes align with the PR objectives of refactoring without introducing functional changes. However, consider the following:
- Ensure that the removal of the postbuild step doesn't omit any critical operations.
- Verify that these changes integrate well with the broader refactoring efforts across the project.
- Update any documentation that might reference the old build process.
To ensure a smooth transition, consider creating or updating a README file in the
packages/themedirectory that explains the new build process and any changes in how developers should work with this package.packages/theme/build/gulp-dist.js (3)
Line range hint
50-54: Fix typo in Autoprefixer options.There's a typo in the Autoprefixer configuration:
'borwsers'should be'browsers'.Apply this diff to correct the typo:
- prefixer({ - borwsers: ['last 1 version', '> 1%', 'not ie <= 8'], + prefixer({ + browsers: ['last 1 version', '> 1%', 'not ie <= 8'],
73-73: Improve the regular expression inreplaceImagesUrl.The current regex
/url\(\.\.\/images\//gmay not cover all possible URL formats, such as URLs with single or double quotes or different whitespace. To make the replacement more robust, consider enhancing the regex.Refactored regex:
- file = file.replace(/url\(\.\.\/images\//g, 'url(images/') + file = file.replace(/url\((['"]?)\.\.\/images\//g, 'url($1images/')This regex accounts for optional quotes around the URL.
78-78: Consistent task definition in thebuildtask.In the
buildtask, some tasks are referenced by name (as strings), and others are referenced directly as functions. For consistency and clarity, consider defining all tasks as functions or all as named tasks.Refactored
buildtask using function references:-gulp.task('build', gulp.series(concatIndex, concatOldTheme, 'compile', 'copyAssets', replaceImagesUrl)) +gulp.task('build', gulp.series(concatIndex, concatOldTheme, compile, copyAssets, replaceImagesUrl))Ensure that
compileandcopyAssetsare exported or defined as functions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (29)
- gulp/buildLowcodeBundle.mjs (0 hunks)
- gulp/buildRenderless.mjs (0 hunks)
- gulp/buildTheme.mjs (0 hunks)
- gulp/buildVue.mjs (0 hunks)
- gulp/themeConcat.mjs (0 hunks)
- gulp/themeJson.alias.mjs (0 hunks)
- gulp/themeJson.mjs (0 hunks)
- gulpfile.mjs (0 hunks)
- package.json (0 hunks)
- packages/theme/build/build-svg-to-css.js (0 hunks)
- packages/theme/build/edit‐dir‐theme.js (0 hunks)
- packages/theme/build/gulp-dist.js (2 hunks)
- packages/theme/build/postbuild.js (0 hunks)
- packages/theme/build/release.js (1 hunks)
- packages/theme/build/replace-img.js (0 hunks)
- packages/theme/package.json (2 hunks)
- packages/theme/scripts/build-component-cssvar.js (0 hunks)
- packages/theme/src/base/basic-var.less (0 hunks)
- packages/theme/src/base/index.less (0 hunks)
- packages/theme/src/base/old-basic-vars.less (0 hunks)
- packages/theme/src/base/old-theme.js (0 hunks)
- packages/theme/src/base/old-theme.less (1 hunks)
- packages/theme/src/base/reset.less (0 hunks)
- packages/theme/src/button/old-theme.less (1 hunks)
- packages/theme/src/card-template/index.less (1 hunks)
- packages/theme/src/index.less (1 hunks)
- packages/theme/src/old-theme.less (1 hunks)
- packages/theme/src/theme-tool.js (1 hunks)
- packages/theme/src/theme.config.js (0 hunks)
💤 Files with no reviewable changes (20)
- gulp/buildLowcodeBundle.mjs
- gulp/buildRenderless.mjs
- gulp/buildTheme.mjs
- gulp/buildVue.mjs
- gulp/themeConcat.mjs
- gulp/themeJson.alias.mjs
- gulp/themeJson.mjs
- gulpfile.mjs
- package.json
- packages/theme/build/build-svg-to-css.js
- packages/theme/build/edit‐dir‐theme.js
- packages/theme/build/postbuild.js
- packages/theme/build/replace-img.js
- packages/theme/scripts/build-component-cssvar.js
- packages/theme/src/base/basic-var.less
- packages/theme/src/base/index.less
- packages/theme/src/base/old-basic-vars.less
- packages/theme/src/base/old-theme.js
- packages/theme/src/base/reset.less
- packages/theme/src/theme.config.js
✅ Files skipped from review due to trivial changes (3)
- packages/theme/src/index.less
- packages/theme/src/old-theme.less
- packages/theme/src/theme-tool.js
🧰 Additional context used
🪛 Biome
packages/theme/build/release.js
[error] 10-11: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 11-12: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (10)
packages/theme/src/button/old-theme.less (1)
1-1: LGTM: Custom stylesheet import.The import of '../custom.less' is correctly formatted and follows good practices for theme customization.
packages/theme/build/release.js (2)
7-9: LGTM: Proper use of asynchronous file operations.The code correctly uses
fs.promisesfor asynchronous file reading, which is a good practice to avoid blocking the event loop.
10-12: 🛠️ Refactor suggestionConsider alternative to delete operator and verify property removal necessity.
While removing 'exports' and 'private' properties might be necessary for the release process, using the delete operator can impact performance. Consider using undefined assignment instead.
Here's a suggested improvement:
-delete packageJson.exports -delete packageJson.private +packageJson.exports = undefined +packageJson.private = undefinedAlso, please verify if removing these properties is necessary for the release process. If you can provide more context on why these properties are being removed, it would be helpful.
To verify the impact of these changes, you can run the following script:
This script will help us understand how these properties are used across the project and assess the potential impact of removing them.
🧰 Tools
🪛 Biome
[error] 10-11: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 11-12: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
packages/theme/package.json (3)
35-35: LGTM: Repositioning of "release" scriptThe "release" script has been repositioned within the "scripts" section. This is a minor change that doesn't affect functionality and is likely due to the removal of other scripts or general file reorganization.
34-34: Verify impact of removing postbuild stepThe simplification of the build script by removing the
node build/postbuild.jsstep aligns with the overall goal of streamlining the build process. This change is generally positive as it reduces complexity.However, we should verify that no critical operations from the removed postbuild step are lost. Let's check the contents of the removed file:
If the file doesn't exist or is empty, we can assume that its functionality has been moved elsewhere or is no longer needed. If it contains significant operations, we may need to ensure those are handled appropriately in the new build process.
✅ Verification successful
Confirmed removal of postbuild step
The
build/postbuild.jsfile does not exist, confirming that removing thepostbuildstep does not omit any critical operations. The build script simplification is safe and approved.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the contents of the removed postbuild.js file # Expected result: If the file exists, we'll see its contents and can assess any important operations that might need to be preserved cat build/postbuild.jsLength of output: 75
3-3: LGTM: Addition of "type": "module"The addition of
"type": "module"is a good practice, indicating that this package should be treated as an ECMAScript module. This aligns with modern JavaScript standards and enables the use of ES modules.However, there's a discrepancy in the AI summary that needs clarification. It mentions both addition and removal of this field. Let's verify if there was any previous declaration:
packages/theme/src/card-template/index.less (2)
Line range hint
1-156: LGTM: Well-structured use of CSS variables for theming.The overall structure of the file is well-organized, with consistent use of CSS variables for styling different parts of the card template. This approach facilitates easy theming and customization.
The use of namespaced classes (e.g.,
@{card-template-prefix-cls}) and CSS variables for various properties (colors, sizes, etc.) is a good practice for maintaining a flexible and maintainable styling system.
20-20: Verify the impact of the new CSS variable injection method.The change from
.component-css-vars-card-template()to.inject-CardTemplate-vars()indicates a shift in how CSS variables are managed for the card template component. This aligns with the broader refactoring of theme tools mentioned in the PR objectives.To ensure this change doesn't introduce any styling regressions:
- Confirm that all necessary variables are properly defined in the new injection method.
- Check for any potential impacts on other components that might have depended on the old variable injection method.
Would you like assistance in reviewing the implementation of
.inject-CardTemplate-vars()to ensure it covers all necessary variables?✅ Verification successful
CSS variable injection method verified successfully.
All instances of
.component-css-vars-card-template()have been removed, and.inject-CardTemplate-vars()is properly defined and utilized within thecard-templatecomponent.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining usage of the old mixin echo "Searching for any remaining usage of .component-css-vars-card-template():" rg ".component-css-vars-card-template\(\)" # Search for the definition of the new mixin echo "Searching for the definition of .inject-CardTemplate-vars():" rg "\.inject-CardTemplate-vars\s*\(\)\s*\{" # List all files that include the new mixin to verify its usage echo "Files using .inject-CardTemplate-vars():" rg -l "\.inject-CardTemplate-vars\(\)"Length of output: 670
packages/theme/build/gulp-dist.js (1)
42-44: Verify inclusion of LESS files to prevent redundancy.Including both
${source}/**/index.lessand${source}/index.lessmight lead to duplicate imports ifindex.lessaggregates the same files. Verify whether both are necessary or if one can be omitted to prevent redundant processing.Run the following script to check for duplicates:
✅ Verification successful
Inclusion of LESS files verified; no duplicate @import statements found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find duplicate @import statements in compiled LESS files. # Test: Search for duplicate imports in index.less. # Expect: No duplicate @import statements. grep "@import" ../src/index.less | sort | uniq -dLength of output: 100
Script:
#!/bin/bash # Description: Locate all index.less files and check for duplicate @import statements. # Find all index.less files in the repository fd --type f --name "index.less" > found_index_less.txt # Check if any index.less files were found if [ ! -s found_index_less.txt ]; then echo "No index.less files found in the repository." exit 0 fi # Iterate over each found index.less file and check for duplicate @import statements while IFS= read -r file; do echo "Checking duplicate @import statements in: $file" grep "@import" "$file" | sort | uniq -d done < found_index_less.txtLength of output: 428
Script:
#!/bin/bash # Description: Locate all index.less files using find and check for duplicate @import statements. # Find all index.less files in the repository find . -type f -name "index.less" > found_index_less.txt # Check if any index.less files were found if [ ! -s found_index_less.txt ]; then echo "No index.less files found in the repository." exit 0 fi # Iterate over each found index.less file and check for duplicate @import statements while IFS= read -r file; do echo "Checking duplicate @import statements in: $file" grep "@import" "$file" | sort | uniq -d done < found_index_less.txtLength of output: 109470
packages/theme/src/base/old-theme.less (1)
1-447: Well-organized and comprehensive theming variablesThe CSS custom properties are clearly defined, consistently named, and cover an extensive range of theming aspects. This enhances maintainability and reusability across the application.
| // 复制package.json/README.md到dist目录 | ||
| fs.copyFileSync('README.md', path.join('dist', 'README.md')) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update comment and consider using asynchronous file operations.
The comment suggests copying both package.json and README.md, but only README.md is being copied. Additionally, consider using asynchronous file operations to avoid blocking the event loop.
Here's a suggested improvement:
-// 复制package.json/README.md到dist目录
-fs.copyFileSync('README.md', path.join('dist', 'README.md'))
+// 复制README.md到dist目录
+await fs.promises.copyFile('README.md', path.join('dist', 'README.md'))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 复制package.json/README.md到dist目录 | |
| fs.copyFileSync('README.md', path.join('dist', 'README.md')) | |
| // 复制README.md到dist目录 | |
| await fs.promises.copyFile('README.md', path.join('dist', 'README.md')) |
| function _concatFiles(srcNames, targetName) { | ||
| const fileList = fg.sync(srcNames) | ||
| const importStr = fileList | ||
| .map((filePath) => filePath.replace('../src/', './')) | ||
| .map((path) => `@import '${path}';`) | ||
| .join('\n') | ||
| fs.writeFileSync(targetName, importStr) | ||
| } |
There was a problem hiding this comment.
Handle errors when writing files in _concatFiles.
The _concatFiles function uses fs.writeFileSync without error handling. If an error occurs during file writing, it could cause the build process to fail silently. Consider adding try-catch blocks or checking for errors to ensure robust error handling.
| // 4、把 src/svgs 复制到 dist/svgs | ||
| gulp.task('copysvgs', () => { | ||
| return gulp.src([`${source}/svgs/**`]).pipe(gulp.dest(`${dist}/svgs`)) | ||
| }) | ||
|
|
||
| // 5、把 src/images 复制到 dist/images | ||
| gulp.task('copyimage', () => { | ||
| return gulp.src([`${source}/images/**`]).pipe(gulp.dest(`${dist}/images`)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Combine copysvgs and copyimage tasks into a single task.
Both tasks copy files from the svgs and images directories to the dist folder. Combining them can simplify the build script and improve maintainability.
Implement the combined task:
gulp.task('copyAssets', () => {
return gulp
.src([`${source}/svgs/**`, `${source}/images/**`], { base: source })
.pipe(gulp.dest(dist));
});Update the build task accordingly:
-gulp.task('build', gulp.series(concatIndex, concatOldTheme, 'compile', 'copysvgs', 'copyimage', replaceImagesUrl))
+gulp.task('build', gulp.series(concatIndex, concatOldTheme, 'compile', 'copyAssets', replaceImagesUrl))| const entryFile = path.join('../dist', 'index.css') | ||
| let file = fs.readFileSync(entryFile, 'utf8') | ||
| file = file.replace(/url\(\.\.\/images\//g, 'url(images/') | ||
| fs.writeFileSync(entryFile, file, 'utf8') | ||
| cb() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use asynchronous file operations in replaceImagesUrl.
Similar to earlier, using synchronous methods can block the event loop. Consider refactoring to use asynchronous methods.
Apply this refactor:
function replaceImagesUrl(cb) {
const entryFile = path.join('../dist', 'index.css');
fs.readFile(entryFile, 'utf8', (err, data) => {
if (err) throw err;
const result = data.replace(/url\((['"]?)\.\.\/images\//g, 'url($1images/');
fs.writeFile(entryFile, result, 'utf8', (err) => {
if (err) throw err;
cb();
});
});
}| .join('\n') | ||
| fs.writeFileSync(targetName, importStr) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using synchronous file operations in an asynchronous context.
Using fs.writeFileSync can block the event loop, which is not ideal in build scripts. It's better to use asynchronous methods like fs.writeFile with callbacks or promises to prevent blocking and improve performance.
Apply this refactor:
- fs.writeFileSync(targetName, importStr)
+ fs.writeFile(targetName, importStr, (err) => {
+ if (err) throw err;
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .join('\n') | |
| fs.writeFileSync(targetName, importStr) | |
| .join('\n') | |
| fs.writeFile(targetName, importStr, (err) => { | |
| if (err) throw err; | |
| }) |
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
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
package.jsonto reflect the latest version and package manager specifications.