refactor(theme-tool): Reimplementing the theme-tool method #2369
Conversation
WalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on the ESLint configuration, Changes
Possibly related PRs
Suggested reviewers
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (6)
packages/theme/src/button/old-theme.less (1)
Line range hint
1-7: Review the theme migration strategy.The change from font-weight to border-radius suggests a significant styling change. Since this is in an "old-theme" file:
- Is there a corresponding change in the new theme files?
- How is backward compatibility being maintained?
- Are there any theme migration guides that need to be updated?
Consider:
- Documenting the theme migration strategy
- Creating a mapping between old and new theme variables
- Adding deprecation warnings if both old and new themes coexist
packages/theme/build/release.js (1)
4-5: Consider using __dirname for more explicit root path resolution.While
path.resolve('./')works, usingpath.resolve(__dirname, '..')would be more explicit and reliable, especially when the script is run from different working directories.-const root = path.resolve('./') +const root = path.resolve(__dirname, '..')packages/theme/src/base/reset.less (1)
Line range hint
16-20: Consider extracting magic numbers to variablesFor better maintainability and consistency, consider:
- Moving the animation duration (2s) to a CSS variable
- Defining the media query breakpoint (768px) as a variable
Here's a suggested improvement:
+ :root { + --tv-animation-duration-loading: 2s; + --tv-breakpoint-tablet: 768px; + } .@{css-prefix-iconfont}-loading { line-height: 1; - animation: rotating 2s linear infinite; + animation: rotating var(--tv-animation-duration-loading) linear infinite; } - @media (min-width: 768px) { + @media (min-width: var(--tv-breakpoint-tablet)) {Also applies to: 134-171
packages/theme/src/old-theme-index.js (2)
1-4: Consider enhancing theme metadata documentation.The theme metadata could benefit from additional documentation to explain:
- The purpose and use cases of this theme
- The relationship between
nameandcnName- Version information or last updated date
Add JSDoc comments above the export:
+/** + * @description Legacy theme configuration for backward compatibility + * @since Version X.X.X + */ export default { id: 'tiny-old-theme', name: 'OldTheme', cnName: '旧的主题',
5-5: Consider implementing a color contrast verification system.The theme defines multiple color combinations for text and backgrounds, but there's no apparent verification of color contrast ratios for accessibility.
Consider implementing a build-time color contrast checker that:
- Extracts color variables
- Calculates contrast ratios for common text/background combinations
- Warns about combinations that don't meet WCAG 2.1 guidelines
Would you like me to create an issue to track this enhancement?
packages/theme/src/theme-tool.js (1)
12-36: Consider adding unit tests for thechangeThememethod.The new
changeThememethod introduces significant functionality. Adding unit tests would ensure that the method behaves as expected under various scenarios and helps prevent future regressions.Would you like assistance in creating unit tests for this method?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- .eslintrc.js (1 hunks)
- .gitignore (2 hunks)
- packages/theme/build/gulp-dist.js (2 hunks)
- packages/theme/build/release.js (1 hunks)
- packages/theme/package.json (1 hunks)
- packages/theme/src/base/index.less (0 hunks)
- packages/theme/src/base/reset.less (1 hunks)
- packages/theme/src/button/old-theme.less (1 hunks)
- packages/theme/src/index.less (0 hunks)
- packages/theme/src/old-theme-index.js (1 hunks)
- packages/theme/src/old-theme.less (0 hunks)
- packages/theme/src/theme-tool.d.ts (1 hunks)
- packages/theme/src/theme-tool.js (1 hunks)
💤 Files with no reviewable changes (3)
- packages/theme/src/base/index.less
- packages/theme/src/index.less
- packages/theme/src/old-theme.less
🧰 Additional context used
🪛 Biome
packages/theme/build/release.js
[error] 31-31: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 31-31: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (11)
packages/theme/src/button/old-theme.less (1)
6-6: Clarify the purpose of modifying a file marked for removal.According to the AI summary, this file (
old-theme.less) is being removed. It seems counterintuitive to add new CSS variables to a file that's scheduled for deletion. This could lead to confusion and potential styling issues.Let's verify the file's status and its dependencies:
packages/theme/build/release.js (1)
11-24: Consider externalizing theme configuration and improving maintainability.
- The theme configuration is hardcoded in the template string. Consider moving it to a separate configuration file.
- Writing to the src directory during build time is unusual and could cause issues with source control.
Let's verify if the src directory is gitignored:
Consider this refactoring:
+const themeConfig = { + id: 'tiny-old-theme', + name: 'OldTheme', + cnName: '旧的主题' +}; + -let jsStr = ` -export default { - id: 'tiny-old-theme', - name: 'OldTheme', - cnName: '旧的主题', - css: \`#CSS#\` -} -` +const jsStr = `export default ${JSON.stringify({ ...themeConfig, css: '#CSS#' }, null, 2)}`; let cssStr = fs.readFileSync(path.resolve(root, 'dist/old-theme-index.css'), 'utf8') jsStr = jsStr.replace('#CSS#', cssStr) -fs.writeFileSync(path.resolve(root, 'src/old-theme-index.js'), jsStr) // 供开发时(pnpm site)， 可以访问到最新的定制主题变量 +// Consider alternative approaches to handle development-time access fs.writeFileSync(path.resolve(root, 'dist/old-theme-index.js'), jsStr).gitignore (2)
71-73: LGTM! Good practice for auto-generated files.Ignoring auto-generated theme files is the correct approach. This prevents merge conflicts and keeps the repository clean.
87-87: LGTM! Appropriate to ignore build artifact.Adding
index.cssto ignored files is correct as it appears to be a build artifact in the examples directory..eslintrc.js (1)
54-54: 🛠️ Refactor suggestionConsider keeping class member spacing for better readability.
Disabling
@typescript-eslint/lines-between-class-membersmight reduce code readability. Blank lines between class members help in visually separating different components of a class, making the code structure clearer.Let's check if this rule needs to be disabled:
Consider either:
- Keeping the rule enabled and adding blank lines between class members for better readability
- If disabling is necessary, use inline
eslint-disablecomments only where needed instead of disabling globallypackages/theme/build/gulp-dist.js (1)
35-35: LGTM! Verify all old-theme files are properly collected.The rename to
old-theme-index.lessbetter reflects its role as an index file.Let's verify all old theme files are properly collected:
✅ Verification successful
Let me gather more information to verify if these are all the expected old-theme files or if we might be missing some components.
Let me check one more thing to understand if this is an intentional migration pattern where only specific components have old themes.
Based on the git history and file analysis, I can now provide a final response:
The old-theme files are correctly collected and intentionally limited to base and button components.
The git history shows this is part of a deliberate theme-tool refactoring where:
- Only
base/old-theme.lessandbutton/old-theme.lessare meant to be included- The
old-theme.lessindex file correctly imports these two specific files- The change from
old-theme.lesstoold-theme-index.lessis consistent with the refactoring🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all old-theme.less files are available to be concatenated # Expected: List of all old-theme.less files that will be concatenated echo "Checking for old-theme.less files in components:" fd "old-theme.less$" "packages/theme/src"Length of output: 228
Script:
#!/bin/bash # Let's check all component directories to ensure we haven't missed any old-theme files echo "All directories under src:" fd --type d --max-depth 1 . "packages/theme/src" # Also check if there are any references to old-theme in less files echo -e "\nChecking for old-theme references in less files:" rg "old-theme" --type less "packages/theme/src" -lLength of output: 5062
Script:
#!/bin/bash # Let's check the git history to understand if this is a migration pattern echo "Checking git history for old-theme.less files:" git log -p --all --full-history -- "**/old-theme.less" # Also check if there's any documentation about the old-theme migration echo -e "\nChecking for migration documentation:" rg -i "old.?theme" -g "!*.{less,js}" --type textLength of output: 54840
packages/theme/package.json (2)
36-36: LGTM! Script reorderingThe release script has been moved but maintains its original functionality.
32-32: Verify the removal of theme directories from clean scriptThe clean script no longer removes
src/aurora-themeandsrc/smb-themedirectories. Please verify:
- If these theme directories have been moved/renamed
- If these directories should be preserved during clean operations
- If build/release processes are affected by this change
✅ Verification successful
Let me check the content of the README files to understand if the theme directories are mentioned as legacy/removed content or if they are still relevant.
Let me check if there are any build or configuration files that might reference these theme directories to understand if they've been moved or renamed.
Let me check if there are any build scripts or configuration files that might reference these theme directories to understand if they're still needed in the build process.
Based on the gathered information, I can now provide the final response:
The removal of aurora-theme and smb-theme directories from clean script is safe
The changes to the clean script are valid because:
- The
src/aurora-themeandsrc/smb-themedirectories no longer exist in the codebase, as evidenced by thesrcdirectory structure- The build system (gulp-dist.js) doesn't reference or depend on these directories
- Theme-related functionality has been refactored to use a different approach, using:
old-theme.lessfiles in component directoriestheme-tool.jsfor dynamic theme switching- A centralized
old-theme-index.jsThe clean script simplification aligns with the current codebase structure and build process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if theme directories still exist and are referenced echo "Checking for theme directory references..." rg -l "aurora-theme|smb-theme" packages/theme/ echo "Checking if theme directories exist..." fd -t d "aurora-theme|smb-theme" packages/theme/src/Length of output: 341
Script:
#!/bin/bash # Check README content for theme directory context echo "Content of README.md:" cat packages/theme/README.md echo -e "\nContent of README.zh-CN.md:" cat packages/theme/README.zh-CN.md # Check git history for theme directory removal echo -e "\nChecking git history for theme directory removal:" git log -p --all -- "packages/theme/src/aurora-theme" "packages/theme/src/smb-theme"Length of output: 825
Script:
#!/bin/bash # Check for build/config files that might reference theme directories echo "Checking build and config files for theme references:" fd -e js -e json -e ts -e vue . packages/theme/ --exec rg -l "aurora-theme|smb-theme" {} # Check if there are any new theme-related directories echo -e "\nListing src directory structure:" tree packages/theme/src/ # Check package.json history for clean script changes echo -e "\nChecking recent changes to package.json:" git log -p packages/theme/package.json | grep -A 5 -B 5 "clean"Length of output: 30196
Script:
#!/bin/bash # Check build files for theme directory references echo "Checking build directory contents:" ls -la packages/theme/build/ echo -e "\nChecking content of gulp build file:" cat packages/theme/build/gulp-dist.js echo -e "\nChecking for any theme-related files in src:" find packages/theme/src/ -type f -name "*theme*"Length of output: 2988
packages/theme/src/base/reset.less (1)
14-14: Verify the existence and compatibility of vars.lessThe import statement has been updated from
basic-var.lesstovars.less. While this change aligns with the theme tool refactoring, we should ensure that all necessary variables are properly defined in the new file.Let's verify the existence and content of both files:
✅ Verification successful
Let me gather more information about the variables being used in reset.less and check vars.less content to ensure compatibility.
This will help us verify if all the variables needed by reset.less are properly defined in vars.less, ensuring the refactoring hasn't broken any dependencies.
Based on the verification results, I can now provide a final response:
Import change to vars.less is valid and safe
The change from
basic-var.lesstovars.lessis correct and properly implemented:
- The new
vars.lessfile exists in the correct location- The old
basic-var.lessfile is no longer present, indicating a complete migration- The
vars.lessfile contains a comprehensive set of CSS variables that define the theme system, including colors, fonts, spacing, shadows, and other design tokens- The
reset.lessfile only uses@importstatements and@keyframes, with no direct usage of variables, making this a safe change🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of both less files and compare their variable definitions # Check if both files exist echo "Checking for the existence of both files..." fd -e less "vars.less|basic-var.less" packages/theme/src/base/ # Compare the variable definitions in both files echo "Checking variable definitions..." rg "^@[^{].*:" packages/theme/src/base/vars.less || true rg "^@[^{].*:" packages/theme/src/base/basic-var.less || trueLength of output: 470
Script:
#!/bin/bash # Check what variables are actually used in reset.less echo "Variables used in reset.less:" rg "@\w+" packages/theme/src/base/reset.less # Check the content of vars.less echo -e "\nContent of vars.less:" cat packages/theme/src/base/vars.lessLength of output: 25506
packages/theme/src/old-theme-index.js (1)
5-6: Verify the button component style scope.The file ends with a specific button component style
.tiny-buttonwhich seems out of place in a theme configuration file. This might lead to specificity issues or unintended style inheritance.Let's check if this button style is used elsewhere or if it should be moved to a component-specific stylesheet:
✅ Verification successful
Let me gather more specific information about the button styles to understand their proper organization.
Based on the results, I can now provide a final response:
The
.tiny-buttonstyle in the theme file is intentional and correctThe single line
.tiny-buttonstyle at the end of the theme file only sets a CSS custom property--tv-Button-border-radius:6pxwhich is then consumed by the actual button component styles defined inpackages/theme/src/button/index.less. This is a proper separation of concerns where:
- The theme file defines the design token (border radius variable)
- The button component's stylesheet consumes this variable through its comprehensive styling system
The button component's actual styles are properly maintained in their dedicated file, and the theme file only provides the necessary design token. This is a common pattern in design systems.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for tiny-button class usage echo "Searching for tiny-button class usage..." rg -l "tiny-button" --type css --type less --type vue # Search for other component-specific styles in theme files echo "Checking for other component styles in theme files..." rg -l "\.tiny-\w+" "packages/theme/src/"Length of output: 1933
Script:
#!/bin/bash # Check button styles in button component echo "Button component styles:" cat packages/theme/src/button/index.less # Check the context around tiny-button in theme file echo -e "\nContext around tiny-button in theme file:" rg -C 5 "\.tiny-button" packages/theme/src/old-theme-index.jsLength of output: 26805
packages/theme/src/theme-tool.js (1)
30-31: Check browser compatibility forCSSStyleSheetandreplaceSync.The
CSSStyleSheetconstructor and thereplaceSyncmethod may not be supported in all browsers. It's advisable to verify browser compatibility and provide fallbacks or polyfills if necessary to ensure consistent behavior across different user environments.
packages/theme/src/theme-tool.d.ts
Outdated
| /** 全局css变量覆盖的对象，比如： { 'tv-base-color-brand' : '#1476ff' }*/ | ||
| data: object |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a more specific type for the data property.
The generic object type provides little type safety. Since this is for CSS variable overrides, consider using a more specific type definition:
- data: object
+ data: Record<string, string>This change would:
- Ensure all keys and values are strings
- Provide better type checking for CSS variable names and values
- Enable better IDE autocompletion
📝 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.
| /** 全局css变量覆盖的对象，比如： { 'tv-base-color-brand' : '#1476ff' }*/ | |
| data: object | |
| /** 全局css变量覆盖的对象，比如： { 'tv-base-color-brand' : '#1476ff' }*/ | |
| data: Record<string, string> |
packages/theme/src/theme-tool.d.ts
Outdated
| changeTheme: (theme: ThemeData) => void | ||
| constructor(theme: ThemeData, styleSheetId: string) | ||
| constructor(theme: ThemeData) |
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered information, I can now generate the final response as there are clear issues that need to be addressed.
Constructor parameter is required but not being provided
The new TinyThemeTool class requires a theme parameter in its constructor, but the current usage in the codebase doesn't provide it:
examples/docs/newsrc/uses/useTheme.js:const theme = new TinyThemeTool()examples/sites/src/tools/useTheme.js:const theme = new TinyThemeTool()
This will cause the constructor to fall back to the default theme, but it's better to explicitly provide the initial theme. Additionally, the implementation in packages/theme-saas/src/theme-tool.js shows that the class still internally uses all the removed properties (styleSheetId, currentTheme, contentElement, defaultTheme), suggesting a mismatch between the type definition and the actual implementation.
Required changes:
- Update
TinyThemeToolinstantiation calls to provide the required theme parameter - Align the implementation with the type definition by properly encapsulating the internal properties
🔗 Analysis chain
LGTM! Verify theme-related functionality remains intact.
The simplified class interface with better encapsulation looks good. The removal of implementation details from the public interface and the addition of the changeTheme method provides a cleaner API.
Let's verify that all theme-related functionality is properly handled:
Let's gather more context about the usage patterns to ensure we have a complete picture.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any direct references to removed properties that might break
rg -l "styleSheetId|currentTheme|contentElement|defaultTheme" --type ts --type js
# Check for proper theme initialization patterns
ast-grep --pattern 'new TinyThemeTool($$$)'
Length of output: 604
Script:
#!/bin/bash
# Check the content of files that initialize TinyThemeTool to see if they're passing required theme parameter
rg -A 5 -B 5 "new TinyThemeTool" examples/docs/newsrc/uses/useTheme.js examples/sites/src/tools/useTheme.js
# Check how the removed properties are being used in the files
rg "styleSheetId|currentTheme|contentElement|defaultTheme" --type ts --type js -C 3
Length of output: 14595
| // 3、复制 package.json | ||
| const content = fs.readFileSync(path.resolve(root, 'package.json'), 'utf8') | ||
| const packageJson = JSON.parse(content) | ||
| delete packageJson.exports | ||
| delete packageJson.private | ||
| await fs.promises.writeFile(path.resolve(root, 'dist/package.json'), JSON.stringify(packageJson, null, 2)) | ||
| fs.writeFileSync(path.resolve(root, 'dist/package.json'), JSON.stringify(packageJson, null, 2)) |
There was a problem hiding this comment.
Add error handling for package.json operations.
While the static analysis flags the delete operator, its usage here is acceptable as this is a build script where performance is not critical, and we're modifying a parsed JSON object, not an object with prototypes.
However, the code should handle potential errors:
-const content = fs.readFileSync(path.resolve(root, 'package.json'), 'utf8')
-const packageJson = JSON.parse(content)
-delete packageJson.exports
-delete packageJson.private
-fs.writeFileSync(path.resolve(root, 'dist/package.json'), JSON.stringify(packageJson, null, 2))
+try {
+ const content = fs.readFileSync(path.resolve(root, 'package.json'), 'utf8')
+ const packageJson = JSON.parse(content)
+ delete packageJson.exports
+ delete packageJson.private
+ fs.writeFileSync(
+ path.resolve(root, 'dist/package.json'),
+ JSON.stringify(packageJson, null, 2)
+ )
+} catch (error) {
+ console.error('Failed to process package.json:', error)
+ process.exit(1)
+}📝 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.
| // 3、复制 package.json | |
| const content = fs.readFileSync(path.resolve(root, 'package.json'), 'utf8') | |
| const packageJson = JSON.parse(content) | |
| delete packageJson.exports | |
| delete packageJson.private | |
| await fs.promises.writeFile(path.resolve(root, 'dist/package.json'), JSON.stringify(packageJson, null, 2)) | |
| fs.writeFileSync(path.resolve(root, 'dist/package.json'), JSON.stringify(packageJson, null, 2)) | |
| // 3、复制 package.json | |
| try { | |
| const content = fs.readFileSync(path.resolve(root, 'package.json'), 'utf8') | |
| const packageJson = JSON.parse(content) | |
| delete packageJson.exports | |
| delete packageJson.private | |
| fs.writeFileSync( | |
| path.resolve(root, 'dist/package.json'), | |
| JSON.stringify(packageJson, null, 2) | |
| ) | |
| } catch (error) { | |
| console.error('Failed to process package.json:', error) | |
| process.exit(1) | |
| } |
🧰 Tools
🪛 Biome
[error] 31-31: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 31-31: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
| // 1、复制 theme-tool.js /README.md 到dist目录 | ||
| fs.copyFileSync('README.md', path.join('dist', 'README.md')) | ||
| fs.copyFileSync('src/theme-tool.js', path.join('dist', 'theme-tool.js')) | ||
| fs.copyFileSync('src/theme-tool.d.ts', path.join('dist', 'theme-tool.d.ts')) |
There was a problem hiding this comment.
Add error handling for file operations.
The synchronous file operations could throw errors if source files don't exist. Consider adding try-catch blocks and file existence checks.
-fs.copyFileSync('README.md', path.join('dist', 'README.md'))
-fs.copyFileSync('src/theme-tool.js', path.join('dist', 'theme-tool.js'))
-fs.copyFileSync('src/theme-tool.d.ts', path.join('dist', 'theme-tool.d.ts'))
+const filesToCopy = [
+ ['README.md', 'dist/README.md'],
+ ['src/theme-tool.js', 'dist/theme-tool.js'],
+ ['src/theme-tool.d.ts', 'dist/theme-tool.d.ts']
+];
+
+filesToCopy.forEach(([src, dest]) => {
+ try {
+ if (!fs.existsSync(src)) {
+ throw new Error(`Source file ${src} does not exist`);
+ }
+ fs.copyFileSync(src, dest);
+ } catch (error) {
+ console.error(`Failed to copy ${src} to ${dest}:`, error);
+ process.exit(1);
+ }
+});📝 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.
| // 1、复制 theme-tool.js /README.md 到dist目录 | |
| fs.copyFileSync('README.md', path.join('dist', 'README.md')) | |
| fs.copyFileSync('src/theme-tool.js', path.join('dist', 'theme-tool.js')) | |
| fs.copyFileSync('src/theme-tool.d.ts', path.join('dist', 'theme-tool.d.ts')) | |
| // 1、复制 theme-tool.js /README.md 到dist目录 | |
| const filesToCopy = [ | |
| ['README.md', 'dist/README.md'], | |
| ['src/theme-tool.js', 'dist/theme-tool.js'], | |
| ['src/theme-tool.d.ts', 'dist/theme-tool.d.ts'] | |
| ]; | |
| filesToCopy.forEach(([src, dest]) => { | |
| try { | |
| if (!fs.existsSync(src)) { | |
| throw new Error(`Source file ${src} does not exist`); | |
| } | |
| fs.copyFileSync(src, dest); | |
| } catch (error) { | |
| console.error(`Failed to copy ${src} to ${dest}:`, error); | |
| process.exit(1); | |
| } | |
| }); |
| # 以下用不到了 | ||
| packages/theme/scripts/theme.json | ||
| packages/theme/scripts/theme-result.txt | ||
| packages/theme/scripts/themeExcel.xlsx | ||
|
|
||
| packages/theme/src/theme/*-theme/component.js | ||
| packages/theme/src/aurora-theme | ||
| packages/theme/src/smb-theme | ||
| #------------------------------ |
There was a problem hiding this comment.
💡 Codebase verification
Theme files have been relocated, not removed
The verification reveals that the theme-related files and directories have been moved from packages/theme/ to packages/theme-saas/:
theme.jsonexists atpackages/theme-saas/theme/theme.json- Aurora and SMB themes exist under
packages/theme-saas/src/theme/
This suggests a restructuring of theme-related code rather than removal. The .gitignore entries should be updated to reflect the new locations if these files still need to be ignored.
Recommended actions:
- Update
.gitignoreentries to point to the new locations underpackages/theme-saas/if these files should still be ignored - Remove the old entries as they no longer reflect the current codebase structure
🔗 Analysis chain
Verify removal of theme-related files from codebase.
The removal of these entries from .gitignore suggests these files are no longer part of the codebase. Let's verify this to prevent potential issues.
Let me generate additional scripts to verify if the theme-related directories and files exist in any form within the repository, as the previous script showed that the specific paths don't exist but we should check if they were moved elsewhere.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the removed theme files still exist in the repository
# Expect: No matches, indicating files have been properly removed
echo "Checking for removed theme files..."
for path in \
"packages/theme/scripts/theme.json" \
"packages/theme/scripts/theme-result.txt" \
"packages/theme/scripts/themeExcel.xlsx" \
"packages/theme/src/aurora-theme" \
"packages/theme/src/smb-theme"; do
if [ -e "$path" ]; then
echo "Warning: $path still exists"
fi
done
# Check for any theme component.js files
fd "component.js" "packages/theme/src/theme" --type f
Length of output: 1473
Script:
#!/bin/bash
# Check if any of these files/patterns exist anywhere in the repository
echo "Searching for theme-related files..."
# Search for theme.json files
echo "=== theme.json files ==="
fd "theme.json" --type f
# Search for theme-result.txt files
echo -e "\n=== theme-result.txt files ==="
fd "theme-result.txt" --type f
# Search for themeExcel.xlsx files
echo -e "\n=== themeExcel.xlsx files ==="
fd "themeExcel.xlsx" --type f
# Search for aurora-theme directory
echo -e "\n=== aurora-theme directories/files ==="
fd "aurora-theme"
# Search for smb-theme directory
echo -e "\n=== smb-theme directories/files ==="
fd "smb-theme"
# Search for component.js files under any theme directory
echo -e "\n=== theme/*-theme/component.js patterns ==="
fd "component.js" --type f | grep -i "theme.*component\.js" || true
Length of output: 884
| @@ -41,7 +41,7 @@ gulp.task('compile', () => { | |||
| .src([ | |||
| `${source}/**/index.less`, // 编译默认主题 | |||
| `${source}/index.less`, | |||
| `${source}/old-theme.less` // 编译旧主题 | |||
| `${source}/old-theme-index.less` // 编译旧主题 | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding source maps for better debugging.
While the path update is correct, the build process could benefit from source maps to aid debugging in development.
Add source map generation to the build pipeline:
gulp.task('compile', () => {
return gulp
.src([
`${source}/**/index.less`,
`${source}/index.less`,
`${source}/old-theme-index.less`
])
+ .pipe(sourcemaps.init())
.pipe(svgInline(svgInlineOption))
.pipe(less())
.pipe(prefixer({
borwsers: ['last 1 version', '> 1%', 'not ie <= 8'],
cascade: true,
remove: true
}))
.pipe(svgInline(svgInlineOption))
.pipe(cssmin())
+ .pipe(sourcemaps.write('./'))
.pipe(gulp.dest(dist))
})This would require adding gulp-sourcemaps to your dependencies:
import sourcemaps from 'gulp-sourcemaps'| id: 'tiny-old-theme', | ||
| name: 'OldTheme', | ||
| cnName: '旧的主题', | ||
| css: `:root{--tv-base-color-brand:#191919;--tv-base-color-brand-1:#f0f7ff;--tv-base-color-brand-2:#deecff;--tv-base-color-brand-3:#b3d6ff;--tv-base-color-brand-4:#7eb7fc;--tv-base-color-brand-5:#4191fa;--tv-base-color-brand-6:#1476ff;--tv-base-color-brand-7:#0f5ed4;--tv-base-color-brand-8:#0845a6;--tv-base-color-brand-9:#022e7a;--tv-base-color-brand-10:#001a4a;--tv-base-color-brand-11:#3d6899;--tv-base-color-brand-12:#7fa6d4;--tv-base-color-brand-13:#b6d4f2;--tv-base-color-common-1:#ffffff;--tv-base-color-common-2:#fafafa;--tv-base-color-common-3:#f5f5f5;--tv-base-color-common-4:#f0f0f0;--tv-base-color-common-5:#e6e6e6;--tv-base-color-common-6:#dbdbdb;--tv-base-color-common-7:#c2c2c2;--tv-base-color-common-8:#808080;--tv-base-color-common-9:#595959;--tv-base-color-common-10:#333333;--tv-base-color-common-11:#191919;--tv-base-color-common-12:#000000;--tv-base-color-success-1:#edf7df;--tv-base-color-success-2:#daf2bb;--tv-base-color-success-3:#b9e683;--tv-base-color-success-4:#9edb58;--tv-base-color-success-5:#7dcc29;--tv-base-color-success-6:#5cb300;--tv-base-color-success-7:#4b9902;--tv-base-color-success-8:#3c8001;--tv-base-color-success-9:#2e6600;--tv-base-color-success-10:#1f4700;--tv-base-color-success-11:#628c38;--tv-base-color-success-12:#a2c777;--tv-base-color-success-13:#d2e6b8;--tv-base-color-success-14:#e6f2d5;--tv-base-color-error-1:#fff1f0;--tv-base-color-error-2:#fce2e0;--tv-base-color-error-3:#faa7a3;--tv-base-color-error-4:#fa8682;--tv-base-color-error-5:#f76360;--tv-base-color-error-6:#f23030;--tv-base-color-error-7:#bf0a1c;--tv-base-color-error-8:#a3171c;--tv-base-color-error-9:#78080e;--tv-base-color-error-10:#4d0005;--tv-base-color-error-11:#a64242;--tv-base-color-error-12:#f2c5c2;--tv-base-color-error-13:#fce3e1;--tv-base-color-warn-1:#fff4e8;--tv-base-color-warn-2:#ffebd1;--tv-base-color-warn-3:#fcd5a4;--tv-base-color-warn-4:#fcbc72;--tv-base-color-warn-5:#ff9a2e;--tv-base-color-warn-6:#ff8800;--tv-base-color-warn-7:#d96900;--tv-base-color-warn-8:#a64d00;--tv-base-color-warn-9:#733400;--tv-base-color-warn-10:#4d2201;--tv-base-color-warn-11:#9e6d3f;--tv-base-color-warn-12:#d6a981;--tv-base-color-warn-13:#f2d8c2;--tv-base-color-warn-secondary-1:#ffb700;--tv-base-color-info-1:#f0f7ff;--tv-base-color-info-2:#deecff;--tv-base-color-info-3:#b3d6ff;--tv-base-color-info-4:#7eb7fc;--tv-base-color-info-5:#4191fa;--tv-base-color-info-6:#1476ff;--tv-base-color-info-7:#0f5ed4;--tv-base-color-info-8:#0845a6;--tv-base-color-info-9:#022e7a;--tv-base-color-info-10:#001a4a;--tv-base-color-info-11:#3d6899;--tv-base-color-info-12:#7fa6d4;--tv-base-color-info-13:#b6d4f2;--tv-color-success-text:var(--tv-base-color-success-6);--tv-color-success-text-primary:var(--tv-base-color-common-11);--tv-color-success-text-white:var(--tv-base-color-common-1);--tv-color-success-bg:var(--tv-base-color-success-6);--tv-color-success-bg-light:var(--tv-base-color-success-14);--tv-color-success-bg-white:var(--tv-base-color-common-1);--tv-color-success-border:var(--tv-base-color-success-6);--tv-color-success-border-light:var(--tv-base-color-success-14);--tv-color-success-icon:var(--tv-base-color-success-6);--tv-color-error-text:var(--tv-base-color-error-6);--tv-color-error-text-primary:var(--tv-base-color-common-11);--tv-color-error-text-white:var(--tv-base-color-common-1);--tv-color-error-bg:var(--tv-base-color-error-6);--tv-color-error-bg-light:var(--tv-base-color-error-13);--tv-color-error-bg-white:var(--tv-base-color-common-1);--tv-color-error-border:var(--tv-base-color-error-6);--tv-color-error-border-light:var(--tv-base-color-error-13);--tv-color-error-icon:var(--tv-base-color-error-6);--tv-color-warn-text:var(--tv-base-color-warn-6);--tv-color-warn-text-primary:var(--tv-base-color-common-11);--tv-color-warn-text-white:var(--tv-base-color-common-1);--tv-color-warn-bg:var(--tv-base-color-warn-6);--tv-color-warn-bg-light:var(--tv-base-color-warn-2);--tv-color-warn-bg-white:var(--tv-base-color-common-1);--tv-color-warn-border:var(--tv-base-color-warn-6);--tv-color-warn-border-light:var(--tv-base-color-warn-2);--tv-color-warn-icon:var(--tv-base-color-warn-6);--tv-color-info-text:var(--tv-base-color-info-6);--tv-color-info-text-primary:var(--tv-base-color-common-11);--tv-color-info-text-white:var(--tv-base-color-common-1);--tv-color-info-bg:var(--tv-base-color-info-6);--tv-color-info-bg-light:var(--tv-base-color-info-2);--tv-color-info-bg-white:var(--tv-base-color-common-1);--tv-color-info-border:var(--tv-base-color-info-6);--tv-color-info-border-light:var(--tv-base-color-info-2);--tv-color-info-icon:var(--tv-base-color-info-6);--tv-color-act-primary-text:var(--tv-base-color-brand);--tv-color-act-primary-text-hover:var(--tv-base-color-brand);--tv-color-act-primary-text-active:var(--tv-base-color-brand);--tv-color-act-primary-text-white:var(--tv-base-color-common-1);--tv-color-act-primary-text-white-hover:var(--tv-base-color-common-1);--tv-color-act-primary-text-white-active:var(--tv-base-color-common-1);--tv-color-act-primary-bg:var(--tv-base-color-brand);--tv-color-act-primary-bg-hover:var(--tv-base-color-common-9);--tv-color-act-primary-bg-active:var(--tv-base-color-common-9);--tv-color-act-primary-bg-white:var(--tv-base-color-common-1);--tv-color-act-primary-bg-white-hover:var(--tv-base-color-common-1);--tv-color-act-primary-bg-white-active:var(--tv-base-color-common-1);--tv-color-act-primary-border:var(--tv-base-color-brand);--tv-color-act-primary-border-hover:var(--tv-base-color-common-9);--tv-color-act-primary-border-active:var(--tv-base-color-common-9);--tv-color-act-primary-border-light:var(--tv-base-color-common-9);--tv-color-act-primary-border-light-hover:var(--tv-base-color-common-1);--tv-color-act-primary-border-light-active:var(--tv-base-color-common-1);--tv-color-act-primary-border-light-hover-1:var(--tv-base-color-common-7);--tv-color-act-primary-border-light-active-1:var(--tv-base-color-common-7);--tv-color-act-success-text:var(--tv-base-color-success-6);--tv-color-act-success-text-hover:var(--tv-base-color-success-6);--tv-color-act-success-text-active:var(--tv-base-color-success-6);--tv-color-act-success-text-white:var(--tv-base-color-common-1);--tv-color-act-success-text-white-hover:var(--tv-base-color-common-1);--tv-color-act-success-text-white-active:var(--tv-base-color-common-1);--tv-color-act-success-bg:var(--tv-base-color-success-6);--tv-color-act-success-bg-hover:var(--tv-base-color-success-5);--tv-color-act-success-bg-active:var(--tv-base-color-success-5);--tv-color-act-success-bg-light:var(--tv-base-color-success-1);--tv-color-act-success-bg-light-hover:var(--tv-base-color-success-1);--tv-color-act-success-bg-light-active:var(--tv-base-color-success-1);--tv-color-act-success-border:var(--tv-base-color-success-6);--tv-color-act-success-border-hover:var(--tv-base-color-success-5);--tv-color-act-success-border-active:var(--tv-base-color-success-5);--tv-color-act-success-border-hover-1:var(--tv-base-color-success-2);--tv-color-act-success-border-active-1:var(--tv-base-color-success-2);--tv-color-act-success-border-light:var(--tv-base-color-success-2);--tv-color-act-success-border-light-hover:var(--tv-base-color-success-1);--tv-color-act-success-border-light-active:var(--tv-base-color-success-1);--tv-color-act-warning-text:var(--tv-base-color-warn-6);--tv-color-act-warning-text-hover:var(--tv-base-color-warn-6);--tv-color-act-warning-text-active:var(--tv-base-color-warn-6);--tv-color-act-warning-text-white:var(--tv-base-color-common-1);--tv-color-act-warning-text-white-hover:var(--tv-base-color-common-1);--tv-color-act-warning-text-white-active:var(--tv-base-color-common-1);--tv-color-act-warning-bg:var(--tv-base-color-warn-6);--tv-color-act-warning-bg-hover:var(--tv-base-color-warn-5);--tv-color-act-warning-bg-active:var(--tv-base-color-warn-5);--tv-color-act-warning-bg-light:var(--tv-base-color-warn-1);--tv-color-act-warning-bg-light-hover:var(--tv-base-color-warn-1);--tv-color-act-warning-bg-light-active:var(--tv-base-color-warn-1);--tv-color-act-warning-border:var(--tv-base-color-warn-6);--tv-color-act-warning-border-hover:var(--tv-base-color-warn-5);--tv-color-act-warning-border-active:var(--tv-base-color-warn-5);--tv-color-act-warning-border-hover-1:var(--tv-base-color-warn-2);--tv-color-act-warning-border-active-1:var(--tv-base-color-warn-2);--tv-color-act-warning-border-light:var(--tv-base-color-warn-2);--tv-color-act-warning-border-light-hover:var(--tv-base-color-warn-1);--tv-color-act-warning-border-light-active:var(--tv-base-color-warn-1);--tv-color-act-danger-text:var(--tv-base-color-error-6);--tv-color-act-danger-text-hover:var(--tv-base-color-error-6);--tv-color-act-danger-text-active:var(--tv-base-color-error-6);--tv-color-act-danger-text-white:var(--tv-base-color-common-1);--tv-color-act-danger-text-white-hover:var(--tv-base-color-common-1);--tv-color-act-danger-text-white-active:var(--tv-base-color-common-1);--tv-color-act-danger-bg:var(--tv-base-color-error-6);--tv-color-act-danger-bg-hover:var(--tv-base-color-error-5);--tv-color-act-danger-bg-active:var(--tv-base-color-error-5);--tv-color-act-danger-bg-light:var(--tv-base-color-error-1);--tv-color-act-danger-bg-light-hover:var(--tv-base-color-error-1);--tv-color-act-danger-bg-light-active:var(--tv-base-color-error-1);--tv-color-act-danger-border:var(--tv-base-color-error-6);--tv-color-act-danger-border-hover:var(--tv-base-color-error-5);--tv-color-act-danger-border-active:var(--tv-base-color-error-5);--tv-color-act-danger-border-hover-1:var(--tv-base-color-error-2);--tv-color-act-danger-border-active-1:var(--tv-base-color-error-2);--tv-color-act-danger-border-light:var(--tv-base-color-error-2);--tv-color-act-danger-border-light-hover:var(--tv-base-color-error-1);--tv-color-act-danger-border-light-active:var(--tv-base-color-error-1);--tv-color-act-info-text:var(--tv-base-color-info-6);--tv-color-act-info-text-hover:var(--tv-base-color-info-6);--tv-color-act-info-text-active:var(--tv-base-color-info-6);--tv-color-act-info-text-white:var(--tv-base-color-common-1);--tv-color-act-info-text-white-hover:var(--tv-base-color-common-1);--tv-color-act-info-text-white-active:var(--tv-base-color-common-1);--tv-color-act-info-bg:var(--tv-base-color-info-6);--tv-color-act-info-bg-hover:var(--tv-base-color-info-5);--tv-color-act-info-bg-active:var(--tv-base-color-info-5);--tv-color-act-info-bg-light:var(--tv-base-color-info-1);--tv-color-act-info-bg-light-hover:var(--tv-base-color-info-1);--tv-color-act-info-bg-light-active:var(--tv-base-color-info-1);--tv-color-act-info-border:var(--tv-base-color-info-6);--tv-color-act-info-border-hover:var(--tv-base-color-info-5);--tv-color-act-info-border-active:var(--tv-base-color-info-5);--tv-color-act-info-border-hover-1:var(--tv-base-color-info-2);--tv-color-act-info-border-active-1:var(--tv-base-color-info-2);--tv-color-act-info-border-light:var(--tv-base-color-info-2);--tv-color-act-info-border-light-hover:var(--tv-base-color-info-1);--tv-color-act-info-border-light-active:var(--tv-base-color-info-1);--tv-color-text:var(--tv-base-color-common-11);--tv-color-text-secondary:var(--tv-base-color-common-9);--tv-color-text-weaken:var(--tv-base-color-common-8);--tv-color-text-placeholder:var(--tv-base-color-common-8);--tv-color-text-disabled:var(--tv-base-color-common-7);--tv-color-text-active:var(--tv-base-color-brand-6);--tv-color-text-important:var(--tv-base-color-common-11);--tv-color-text-white:var(--tv-base-color-common-1);--tv-color-text-link:var(--tv-base-color-brand-6);--tv-color-text-link-hover:var(--tv-base-color-brand-6);--tv-color-icon:var(--tv-base-color-common-8);--tv-color-icon-hover:var(--tv-base-color-common-11);--tv-color-icon-active:var(--tv-base-color-common-11);--tv-color-icon-disabled:var(--tv-base-color-common-7);--tv-color-icon-checked-disabled:var(--tv-base-color-common-6);--tv-color-icon-control:var(--tv-base-color-brand);--tv-color-icon-control-active:var(--tv-base-color-brand-6);--tv-color-icon-control-disabled:var(--tv-base-color-common-6);--tv-color-icon-white:var(--tv-base-color-common-1);--tv-color-icon-link:var(--tv-base-color-brand-6);--tv-color-icon-warn-secondary:var(--tv-base-color-warn-secondary-1);--tv-color-bg:var(--tv-base-color-common-3);--tv-color-bg-primary:var(--tv-base-color-brand);--tv-color-bg-secondary:var(--tv-base-color-common-1);--tv-color-bg-control:var(--tv-base-color-common-7);--tv-color-bg-control-unactive:var(--tv-base-color-common-7);--tv-color-bg-gray-1:var(--tv-base-color-common-2);--tv-color-bg-gray-2:var(--tv-base-color-common-5);--tv-color-bg-header:var(--tv-base-color-common-4);--tv-color-bg-mask:rgba(0, 0, 0, 0.3);--tv-color-bg-disabled:var(--tv-base-color-common-4);--tv-color-bg-disabled-control-checked:var(--tv-base-color-common-6);--tv-color-bg-disabled-control-unactive:var(--tv-base-color-common-6);--tv-color-bg-disabled-control-active:var(--tv-base-color-brand-3);--tv-color-bg-hover:var(--tv-base-color-common-3);--tv-color-bg-hover-primary:var(--tv-base-color-common-9);--tv-color-bg-hover-secondary:var(--tv-base-color-common-1);--tv-color-bg-hover-1:var(--tv-base-color-brand-2);--tv-color-bg-hover-2:var(--tv-base-color-common-5);--tv-color-bg-hover-3:var(--tv-base-color-brand-1);--tv-color-bg-active:var(--tv-base-color-common-3);--tv-color-bg-active-control:var(--tv-base-color-brand-6);--tv-color-bg-active-primary:var(--tv-base-color-brand);--tv-color-bg-active-secondary:var(--tv-base-color-common-1);--tv-color-bg-active-emphasize:var(--tv-base-color-brand-2);--tv-color-bg-active-emphasize-light:var(--tv-base-color-brand-1);--tv-color-border:var(--tv-base-color-common-7);--tv-color-border-hover:var(--tv-base-color-brand);--tv-color-border-active:var(--tv-base-color-brand);--tv-color-border-active-control:var(--tv-base-color-brand-6);--tv-color-border-disabled:var(--tv-base-color-common-6);--tv-color-border-secondary:var(--tv-base-color-common-9);--tv-color-border-ghost:var(--tv-base-color-common-8);--tv-color-border-ghost-active:var(--tv-base-color-common-6);--tv-color-border-divider:var(--tv-base-color-common-4);--tv-color-border-divider-short:var(--tv-base-color-common-6);--tv-font-family:'Helvetica','Arial','PingFangSC-Regular','Hiragino Sans GB','Microsoft YaHei','微软雅黑','Microsoft JhengHei';--tv-font-family-1:'Arial','San Francisco','Helvetica';--tv-font-size-sm:12px;--tv-font-size-md:14px;--tv-font-size-lg:16px;--tv-font-size-xl:18px;--tv-font-size-xxl:20px;--tv-font-size-heading-xs:16px;--tv-font-size-heading-sm:18px;--tv-font-size-heading-md:20px;--tv-font-size-heading-lg:24px;--tv-font-size-heading-xl:32px;--tv-line-height-number:1.5;--tv-font-weight-thin:200;--tv-font-weight-regular:400;--tv-font-weight-bold:600;--tv-border-radius-xs:2px;--tv-border-radius-sm:4px;--tv-border-radius-md:6px;--tv-border-radius-lg:8px;--tv-border-radius-round:999px;--tv-border-width:1px;--tv-space-base:4px;--tv-space-xs:2px;--tv-space-sm:var(--tv-space-base);--tv-space-md:calc(var(--tv-space-base) * 2);--tv-space-lg:calc(var(--tv-space-base) * 3);--tv-space-xl:calc(var(--tv-space-base) * 4);--tv-size-base:4px;--tv-size-height-xs:24px;--tv-size-height-sm:28px;--tv-size-height-md:32px;--tv-size-height-lg:40px;--tv-size-height-xl:48px;--tv-icon-size:16px;--tv-shadow-0:0 4px 16px 0 rgba(0, 0, 0, 0.08);--tv-shadow-1:0 0 0 0 rgba(0, 0, 0, 0);--tv-shadow-1-up:0 -1px 4px 0 rgba(0, 0, 0, 0.08);--tv-shadow-1-down:0 1px 4px 0 rgba(0, 0, 0, 0.08);--tv-shadow-1-left:-1px 0 4px 0 rgba(0, 0, 0, 0.08);--tv-shadow-1-right:1px 0 4px 0 rgba(0, 0, 0, 0.08);--tv-shadow-2-up:0 -2px 12px 0 rgba(0, 0, 0, 0.16);--tv-shadow-2-down:0 2px 12px 0 rgba(0, 0, 0, 0.16);--tv-shadow-2-left:-2px 0 12px 0 rgba(0, 0, 0, 0.16);--tv-shadow-2-right:2px 0 12px 0 rgba(0, 0, 0, 0.16);--tv-shadow-3-up:0 -4px 16px 0 rgba(0, 0, 0, 0.08);--tv-shadow-3-down:0 4px 16px 0 rgba(0, 0, 0, 0.08);--tv-shadow-3-left:-4px 0 16px 0 rgba(0, 0, 0, 0.08);--tv-shadow-3-right:4px 0 16px 0 rgba(0, 0, 0, 0.08);--tv-shadow-4-up:0 -8px 24px 0 rgba(0, 0, 0, 0.16);--tv-shadow-4-down:0 8px 24px 0 rgba(0, 0, 0, 0.16);--tv-shadow-4-left:-8px 0 24px 0 rgba(0, 0, 0, 0.16);--tv-shadow-4-right:8px 0 24px 0 rgba(0, 0, 0, 0.16);--tv-size-scrollbar-width:8px;--tv-size-scrollbar-height:8px;--tv-border-radius-scrollbar-thumb:4px;--tv-color-bg-scrollbar-thumb:var(--tv-base-color-common-6);--tv-color-bg-scrollbar-thumb-hover:var(--tv-base-color-common-6);--tv-color-bg-scrollbar-thumb-active:var(--tv-base-color-common-6);--tv-color-bg-scrollbar-track:var(--tv-base-color-common-1)}.tiny-button{--tv-Button-border-radius:6px}` |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing CSS custom properties fallback values.
The CSS variables don't have fallback values, which could cause styling issues if any variable is undefined.
Add fallback values for critical properties:
css: `:root{
- --tv-color-text:var(--tv-base-color-common-11);
+ --tv-color-text:var(--tv-base-color-common-11, #333333);
- --tv-font-size-md:14px;
+ --tv-font-size-md:var(--tv-base-font-size, 14px);
}`Committable suggestion was skipped due to low confidence.
🛠️ Refactor suggestion
Consider splitting CSS variables into logical groups.
The CSS variables are currently defined in a single large string, which could make maintenance challenging. Consider splitting them into logical groups using CSS comments for better organization.
Add section comments to improve readability:
css: `:root{
+ /* Base Colors */
--tv-base-color-brand:#191919;
...
+ /* Typography */
--tv-font-family:'Helvetica',...;
...
+ /* Spacing & Sizing */
--tv-space-base:4px;
...
+ /* Shadows */
--tv-shadow-0:0 4px 16px 0 rgba(0, 0, 0, 0.08);
...
}`Committable suggestion was skipped due to low confidence.
packages/theme/src/theme-tool.js
Outdated
| if (this._loadedSheet) { | ||
| document.adoptedStyleSheets = document.adoptedStyleSheets.filter((t) => t !== this._loadedSheet) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling browsers that do not support adoptedStyleSheets.
The use of document.adoptedStyleSheets is not supported in all browsers, such as Internet Explorer and some versions of Safari. To enhance compatibility, consider adding a fallback for browsers that do not support this API.
| if (theme.css && typeof theme.css === 'string') { | ||
| cssContent += theme.css | ||
| } |
There was a problem hiding this comment.
Ensure theme.css is from a trusted source to prevent CSS injection.
Appending theme.css directly to cssContent could introduce security risks if theme.css contains malicious code. Ensure that theme.css is from a trusted source or implement sanitization to prevent CSS injection attacks.
| cssContent = Object.keys(theme.data) | ||
| .map((key) => `--${key}: ${theme.data[key]}; `) | ||
| .join('') |
There was a problem hiding this comment.
Sanitize theme data to prevent CSS injection vulnerabilities.
Currently, the keys and values from theme.data are directly inserted into CSS without sanitization. This could lead to CSS injection if theme.data contains malicious content. Consider using CSS.escape() to sanitize both the keys and values.
Apply this diff to sanitize the keys and values:
cssContent = Object.keys(theme.data)
- .map((key) => `--${key}: ${theme.data[key]}; `)
+ .map((key) => `--${CSS.escape(key)}: ${CSS.escape(theme.data[key])}; `)
.join('');📝 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.
| cssContent = Object.keys(theme.data) | |
| .map((key) => `--${key}: ${theme.data[key]}; `) | |
| .join('') | |
| cssContent = Object.keys(theme.data) | |
| .map((key) => `--${CSS.escape(key)}: ${CSS.escape(theme.data[key])}; `) | |
| .join('') |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
packages/theme/src/theme-tool.d.ts (2)
17-22: Update JSDoc example to show proper usage.The example should demonstrate the recommended usage pattern with a theme parameter.
Update the example to:
/** * 动态切换文档或影子根节点的样式类 * @example * const theme = { name: 'light', data: { '--tv-base-color-brand': '#1476ff' } }; * const themeTool = new TinyThemeTool(theme); * themeTool.changeTheme(theme) */
31-31: Specify return type for changeTheme method.The method's return type is implicitly void. It's better to be explicit for better API documentation.
Update the method signature to:
changeTheme: (theme: ThemeData, target?: Document | ShadowRoot) => void;packages/theme/src/theme-tool.js (3)
1-3: Add JSDoc documentation for the exported OldTheme.Consider adding documentation to explain the purpose and usage of the
OldThemeexport, especially since it appears to be part of a refactoring effort.+/** + * @deprecated Use TinyThemeTool for new implementations + * Represents the old theme configuration for backward compatibility + */ export { OldTheme }
5-7: Translate JSDoc comments to English.For better international collaboration, consider translating the documentation to English.
-/** - * 动态切换文档或影子根节点的样式类 - */ +/** + * Dynamically switches styles for document or shadow root nodes + */
9-14: Consider adding a cleanup method for loaded styles.The
loadedcache might grow indefinitely if themes are changed frequently across many targets. Consider adding a cleanup method to remove unused stylesheets.constructor(theme) { this.loaded = {} // 缓存已加载的样式 if (theme) { this.changeTheme(theme) } } + +/** + * Removes cached stylesheet for a specific target + * @param {Document|ShadowRoot} target - The target to cleanup + */ +cleanup(target) { + if (this.loaded[target]) { + const index = target.adoptedStyleSheets.indexOf(this.loaded[target]); + if (index > -1) { + target.adoptedStyleSheets.splice(index, 1); + } + delete this.loaded[target]; + } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/theme/src/theme-tool.d.ts (1 hunks)
- packages/theme/src/theme-tool.js (1 hunks)
🔇 Additional comments (2)
packages/theme/src/theme-tool.d.ts (1)
24-24: Verify implementation alignment with type definition.The constructor signature has been simplified, but we should verify that the implementation properly handles the optional theme parameter.
✅ Verification successful
Let me gather more specific information about the constructor implementation.
Constructor implementation aligns with type definition
The implementation correctly handles the optional theme parameter:
- It initializes internal state regardless of theme parameter
- Uses an if-check to conditionally call
changeThemeonly when theme is provided- This matches the
theme?: ThemeDataoptional parameter in the type definition🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check the implementation to ensure it properly handles optional theme parameter rg -l "class TinyThemeTool" packages/theme/src/ ast-grep --pattern 'class TinyThemeTool { constructor($$$) { $$$ } }'Length of output: 197
Script:
#!/bin/bash # Get the actual implementation of TinyThemeTool constructor rg -A 10 "class TinyThemeTool" packages/theme/src/theme-tool.jsLength of output: 324
packages/theme/src/theme-tool.js (1)
24-35:⚠️ Potential issuePrevious security concerns about CSS injection are still valid.
The code still directly injects values from
theme.dataandtheme.csswithout sanitization, which could lead to CSS injection vulnerabilities.if (theme && (theme.data || theme.css)) { let cssContent = '' if (theme.data && typeof theme.data === 'object') { cssContent = Object.keys(theme.data) - .map((key) => `--${key}: ${theme.data[key]}; `) + .map((key) => `--${CSS.escape(key)}: ${CSS.escape(String(theme.data[key]))}; `) .join('') cssContent = `:root{${cssContent}}` } if (theme.css && typeof theme.css === 'string') { + // Consider using a CSS sanitizer library here + if (!/^[^<>]*$/.test(theme.css)) { + throw new Error('Invalid characters detected in theme.css'); + } cssContent += theme.css }
| /** | ||
| * 需要追加的全局css变量的对象。 | ||
| * 比如： { 'tv-base-color-brand' : '#1476ff' } 会追加到 :root { --tv-base....... } | ||
| * */ | ||
| data?: Record<string, string> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding type safety for CSS variable names.
While Record<string, string> is better than object, we can further improve type safety for CSS variables.
Consider creating a more specific type for CSS variable names:
type CSSVariableName = `--tv-${string}`
interface ThemeData {
// ...
data?: Record<CSSVariableName, string>
// ...
}This ensures that all variable names follow the --tv- prefix convention shown in the example.
| changeTheme(theme, target = document) { | ||
| let loadedSheet = this.loaded[target] | ||
| if (!loadedSheet) { | ||
| loadedSheet = new CSSStyleSheet() | ||
| target.adoptedStyleSheets = [...target.adoptedStyleSheets, loadedSheet] | ||
| this.loaded[target] = loadedSheet | ||
| } |
There was a problem hiding this comment.
Add browser compatibility check and fallback.
The adoptedStyleSheets API isn't supported in all browsers. Add a fallback mechanism using traditional stylesheet injection.
changeTheme(theme, target = document) {
+ if (!('adoptedStyleSheets' in target)) {
+ // Fallback for browsers not supporting adoptedStyleSheets
+ let styleElement = target._tinyThemeStyle;
+ if (!styleElement) {
+ styleElement = document.createElement('style');
+ target._tinyThemeStyle = styleElement;
+ (target.head || target).appendChild(styleElement);
+ }
+ this.loaded[target] = styleElement;
+ return this._applyThemeClassic(theme, styleElement);
+ }
+
let loadedSheet = this.loaded[target]
if (!loadedSheet) {
loadedSheet = new CSSStyleSheet()
target.adoptedStyleSheets = [...target.adoptedStyleSheets, loadedSheet]
this.loaded[target] = loadedSheet
}Committable suggestion was skipped due to low confidence.
| changeTheme(theme, target = document) { | ||
| let loadedSheet = this.loaded[target] | ||
| if (!loadedSheet) { | ||
| loadedSheet = new CSSStyleSheet() | ||
| target.adoptedStyleSheets = [...target.adoptedStyleSheets, loadedSheet] | ||
| this.loaded[target] = loadedSheet | ||
| } | ||
|
|
||
| if (theme && (theme.data || theme.css)) { | ||
| let cssContent = '' | ||
| if (theme.data && typeof theme.data === 'object') { | ||
| cssContent = Object.keys(theme.data) | ||
| .map((key) => `--${key}: ${theme.data[key]}; `) | ||
| .join('') | ||
|
|
||
| cssContent = `:root{${cssContent}}` | ||
| } | ||
| if (theme.css && typeof theme.css === 'string') { | ||
| cssContent += theme.css | ||
| } | ||
|
|
||
| loadedSheet.replaceSync(cssContent) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type validation for theme object.
The method should validate the theme object structure to prevent runtime errors and improve debugging.
+/**
+ * @typedef {Object} Theme
+ * @property {Record<string, string|number>} [data] - Theme CSS variables
+ * @property {string} [css] - Raw CSS content
+ */
+
+/**
+ * Changes the theme for a target
+ * @param {Theme} theme - The theme configuration
+ * @param {Document|ShadowRoot} target - The target to apply theme to
+ * @throws {Error} If theme object is invalid
+ */
changeTheme(theme, target = document) {
+ if (theme && typeof theme !== 'object') {
+ throw new TypeError('Theme must be an object');
+ }
+ if (theme?.data && typeof theme.data !== 'object') {
+ throw new TypeError('Theme.data must be an object');
+ }
+ if (theme?.css && typeof theme.css !== 'string') {
+ throw new TypeError('Theme.css must be a string');
+ }Committable suggestion was skipped due to low confidence.
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
Release Notes
TinyThemeToolclass.old-theme-index.js..gitignoreto clean up ignored files.