refactor(steps): [steps, timeline] Refactoring the steps and timeline themes #2261
Conversation
WalkthroughThe pull request introduces several modifications to the 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (12)
packages/theme/src/steps/aurora-theme.js (3)
2-39: Approve naming convention changes and suggest documentation update.The renaming of all properties from
ti-steps-*totv-Steps-*improves consistency with what appears to be a new naming convention. This change enhances clarity and maintainability.Consider updating any relevant documentation or style guides to reflect this new naming convention, ensuring that other developers are aware of the change and can maintain consistency in future updates.
8-8: Approve formatting and suggest comment improvement.The overall formatting of the theme object is consistent and readable. The comment on line 8 about not omitting 'px' is helpful.
Consider expanding the comment to explain why 'px' cannot be omitted. For example:
'tv-Steps-line-margin-horizontal': '0px', // 'px' is required here as some browsers don't recognize unitless zero values for certain propertiesThis additional context can help prevent future developers from accidentally removing the unit.
1-39: Summary of changes and final suggestions.Overall, the changes to the
tinyStepsAuroraThemeobject improve consistency and clarity. The new naming convention with thetv-Steps-prefix is well-applied throughout the theme. Here are the key points and suggestions from this review:
- The naming convention change is approved and consistent.
- Consider using CSS variables for the remaining hardcoded color values to improve maintainability.
- The formatting is consistent and readable.
- The comment about 'px' units is helpful but could be expanded for clarity.
To further improve this theme:
- Ensure that the new naming convention is documented in the project's style guide.
- Review the use of hardcoded color values and consider creating a color palette with CSS variables.
- Add a brief comment at the top of the file explaining the purpose of this theme and how it relates to the Steps component.
These changes will enhance the maintainability and clarity of the Aurora theme for the Steps component.
examples/sites/demos/apis/steps.js (4)
22-22: LGTM! Consider updating the description for clarity.The changes to the
advancedproperty look good:
- The type change from
Booleantobooleanaligns with TypeScript conventions.- Expanding the mode to include 'pc' is consistent with the new functionality.
Consider updating the description to reflect that this feature is now available for both mobile and PC modes:
desc: { - 'zh-CN': '是否开启高级向导模式', - 'en-US': 'Enable Advanced Wizard Mode' + 'zh-CN': '是否开启高级向导模式（适用于移动端和PC端）', + 'en-US': 'Enable Advanced Wizard Mode (applicable for both mobile and PC)' },Also applies to: 28-30
103-113: LGTM! Consider adding a note about mobile compatibility.The addition of the
lineproperty is well-documented and seems to provide useful functionality for horizontal single chain step bars in PC mode.Consider adding a note in the description about its compatibility with mobile mode:
desc: { 'zh-CN': '通过 line 设置横向单链型步骤条', - 'en-US': 'Set horizontal single chain step bar through line' + 'en-US': 'Set horizontal single chain step bar through line (PC mode only)' },
Line range hint
129-129: Consider usingfalseas the default value forno-arrow.The current change sets the default value to an empty string, which might be confusing for a boolean property.
Consider changing the default value to
falsefor clarity:- defaultValue: '', + defaultValue: 'false',This change would make it clear that arrows are shown by default.
Line range hint
140-141: LGTM for type change. Consider improvements to default value and description.The change from
Stringtostringfor thesizeproperty type is correct and aligns with TypeScript conventions.
- Consider setting a specific default value instead of an empty string:
- defaultValue: '', + defaultValue: "'medium'",
- Update the description to clarify the default values for different modes:
desc: { 'zh-CN': - 'line 单链型模式支持 mini、small、medium、large 4 种尺寸，默认值为 medium。advanced 高级向导模式支持 medium、large 2 种尺寸，默认值为 medium', + 'line 单链型模式支持 mini、small、medium、large 4 种尺寸，默认值为 medium。advanced 高级向导模式支持 medium、large 2 种尺寸，默认值为 medium。若未指定，默认为 medium。', - 'en-US': '' + 'en-US': 'In line mode, supports 4 sizes: mini, small, medium, large (default: medium). In advanced mode, supports 2 sizes: medium, large (default: medium). If not specified, defaults to medium.' },packages/theme/src/steps/vars.less (3)
14-254: Consider translating comments to English for consistencyThe code contains comments in Chinese (e.g., lines 14, 86, 117, 152). To maintain consistency and improve accessibility for all team members, please consider translating all comments into English.
78-84: Standardize naming conventions for size variablesThere are inconsistent naming conventions for size-related variables. For example:
--tv-Steps-mini-icon-size--tv-Steps-mini-number-font-size--tv-Steps-icon-small-size--tv-Steps-small-number-font-sizeConsider standardizing the naming to maintain consistency. For instance, you could use
--tv-Steps-icon-size-miniand--tv-Steps-icon-size-smallfor better uniformity.
228-236: Use consistent terminology for status variablesThe variables for status colors use both "danger" and "error" interchangeably:
--tv-Steps-timeline-dot-danger-bg-color--tv-Steps-error-icon-color--tv-Steps-advance-node-error-bg-colorConsider standardizing the terminology to either "error" or "danger" across all variables to improve code clarity and consistency.
packages/theme/src/steps/index.less (2)
233-233: Use consistent spacing for icon marginsThe margin-right for
.@{steps-prefix-cls}-iconis set to8px. Review if this aligns with the spacing conventions used elsewhere in the project to ensure visual consistency.
Line range hint
90-93: Remove redundantmargin-rightproperty in vertical layoutIn the
.verticalclass, bothtext-align: center;andmargin-right: 0;are set. If centering the content negates the need for adjusting the margin, consider removing the redundantmargin-rightproperty to simplify the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- examples/sites/demos/apis/steps.js (2 hunks)
- examples/sites/demos/pc/app/steps/advanced-steps-composition-api.vue (1 hunks)
- packages/theme/src/steps/aurora-theme.js (1 hunks)
- packages/theme/src/steps/index.less (22 hunks)
- packages/theme/src/steps/old-theme.js (1 hunks)
- packages/theme/src/steps/vars.less (1 hunks)
- packages/theme/src/theme/infinity-theme/component-vars.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (16)
examples/sites/demos/pc/app/steps/advanced-steps-composition-api.vue (2)
Line range hint
1-35: Overall, the changes align with the refactoring objectives.The modifications to the step statuses and count in the
dataref appear to be intentional and part of the refactoring process for the steps theme. The rest of the component, including the template and theadvancedClickfunction, remains unchanged, which is consistent with the PR's goal of not introducing functional changes.However, please ensure that these status changes are reflected in any related documentation or test cases to maintain consistency across the project.
18-22: Verify the impact of status and count changes on the steps component.The changes to the
dataref modify the status and count of various steps:
- "Basic Info" is now marked as 'done' instead of 'doing'.
- "BOQ Info" is changed to 'doing' from 'done'.
- "Involved Parties" now has an 'error' status instead of 'doing'.
- "Billing" is set to 'disabled' from 'done'.
- The count for "Appraise" is increased from 0 to 3.
These modifications will affect the visual representation and potentially the behavior of the steps component. Please ensure that these changes align with the intended refactoring goals and don't introduce unexpected side effects in other parts of the application that might depend on these statuses.
To verify the impact of these changes, you can run the following script:
This script will help identify any potential areas in the codebase that might be affected by these status changes.
packages/theme/src/steps/old-theme.js (6)
2-9: LGTM: Consistent renaming of timeline-related properties.The renaming from 'ti-steps-' to 'tv-Steps-' for timeline-related properties is consistent and maintains the existing functionality. The values correctly reference the appropriate CSS variables.
10-15: Verify the change from 'advanced' to 'advance' in property names.While the renaming from 'ti-steps-' to 'tv-Steps-' is consistent, there's a change from 'advanced' to 'advance' in the following properties:
- 'tv-Steps-advance-count-font-size'
- 'tv-Steps-advance-count-bg-color'
- 'tv-Steps-advance-link-font-size'
- 'tv-Steps-advance-li-text-color'
This change might be unintentional and could lead to confusion. Please verify if this change is intended or if it should be reverted to 'advanced'.
16-21: LGTM: Improved specificity in disabled and error state property names.The renaming of disabled and error state properties is consistent and provides more clarity on the specific elements being styled. For example:
- 'tv-Steps-line-number-active-text-color-disabled' is more specific than the previous naming.
- 'tv-Steps-line-title-text-color-disabled' clearly indicates it's for the line title.
This improved specificity will make it easier for developers to understand and use these theme properties.
22-31: LGTM: Enhanced clarity in active, done, and hover state property names.The renaming of active, done, and hover state properties is consistent and offers improved clarity:
- 'tv-Steps-line-doing-border-color' clearly indicates it's for the line in the 'doing' state.
- 'tv-Steps-line-desc-text-color-active' specifies it's for the description text color in the active state.
These changes will help developers more easily identify and apply the correct styles for different states of the steps component.
32-42: LGTM: Improved semantic naming for various properties.The renaming of these properties enhances clarity and semantic meaning:
- 'tv-Steps-custom-icon-size' replaces separate width and height properties, assuming icons are square.
- 'tv-Steps-node-circle-size' is more semantically correct than 'circle-width-height' for a single size value.
- Other properties maintain consistency with the new naming convention while preserving their original meanings.
These changes contribute to a more intuitive and maintainable theme structure.
1-42: Overall: Consistent renaming improves theme structure with minor concerns.The changes in this file demonstrate a systematic renaming of all properties in the
tinyStepsOldThemeobject from 'ti-steps-' to 'tv-Steps-'. This renaming:
- Improves consistency across the theme.
- Enhances clarity with more specific naming for certain properties.
- Maintains the existing functionality by preserving the CSS variable references.
However, there are two points that require attention:
- The change from 'advanced' to 'advance' in some property names (lines 10-13) needs verification.
- The rationale behind the prefix change from 'ti-' to 'tv-' is not clear from this file alone and might benefit from a comment explaining the change.
These changes will likely improve the maintainability of the theme, but ensuring consistency with the rest of the codebase is crucial.
examples/sites/demos/apis/steps.js (2)
Line range hint
60-60: LGTM! Type correction improves consistency.The change from
Stringtostringfor thedescription-fieldproperty type is correct and aligns with TypeScript conventions.
Line range hint
1-285: Overall, the changes improve the API definition, but there's room for further refinement.The modifications to the
stepscomponent API enhance type consistency and expand functionality. Key improvements include:
- Consistent use of lowercase
booleanandstringtypes.- Expansion of the
advancedproperty to PC mode.- Addition of the
lineproperty for horizontal single chain step bars.Consider implementing the suggested improvements to default values and descriptions for better clarity and usability.
packages/theme/src/theme/infinity-theme/component-vars.js (1)
221-227: Consistent renaming of Steps component CSS variablesThe changes in these lines represent a systematic renaming of CSS variables for the Steps component. Here's a summary of the changes:
- The prefix has changed from 'ti-' to 'tv-' for all variables.
- 'steps' has been capitalized to 'Steps' in all variable names.
- Some variable names have been slightly modified:
- 'done-icon-bg-color' → 'node-done-bg-color'
- 'done-active-border-color' → 'node-done-border-color'
- All values remain unchanged.
These changes appear to be part of a larger effort to standardize naming conventions across the codebase. The new naming scheme seems more consistent and descriptive.
To ensure these changes don't break existing styles, we should verify that all references to these variables have been updated accordingly. Run the following script to check for any remaining occurrences of the old variable names:
If this script returns any results, those occurrences will need to be updated to use the new variable names.
✅ Verification successful
No remaining occurrences of old Steps component CSS variables found
All instances of the old
ti-steps-variable names have been successfully updated to the newtv-Steps-naming convention. No references toti-steps-variables were found in.css,.js, or.vuefiles.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining occurrences of the old variable names echo "Searching for old variable names:" rg --type css --type vue --type js 'ti-steps-(icon-size|done-icon-bg-color|done-active-border-color|done-icon-fill-color|line-active-bg-color|line-height|done-text-color)'Length of output: 278
Script:
#!/bin/bash # Search for any remaining occurrences of the old variable names in .css, .js, and .vue files echo "Searching for old variable names in .css, .js, and .vue files:" rg 'ti-steps-(icon-size|done-icon-bg-color|done-active-border-color|done-icon-fill-color|line-active-bg-color|line-height|done-text-color)' --type css --type js --type vueLength of output: 336
Script:
#!/bin/bash # Search for any remaining occurrences of the old variable names in .css, .js, and .vue files without using --type flags echo "Searching for old variable names in .css, .js, and .vue files:" rg 'ti-steps-(icon-size|done-icon-bg-color|done-active-border-color|done-icon-fill-color|line-active-bg-color|line-height|done-text-color)' -g '*.css' -g '*.js' -g '*.vue'Length of output: 308
packages/theme/src/steps/index.less (5)
183-185: Confirm the correct positioning of arrow elementsThe
.arrowclass usesposition: absolutewith specific dimensions. Ensure that the arrow elements are properly positioned across different browsers and screen sizes to maintain the intended design.
1191-1194: Consistent styling for.process-current .iconThe styles applied to
.process-current .iconshould align with the overall design system. Verify that the colors and backgrounds used here are consistent with other states and components.
545-545: Potential duplication of review commentThis comment appears to duplicate a previous comment regarding the
--tv-Steps-error-text-colorvariable.
545-545: Check the definition of--tv-Steps-error-text-colorvariableVerify that the CSS variable
--tv-Steps-error-text-colorused in line 545 is correctly defined in the theme variables. An undefined variable may result in unexpected styling issues.Run the following script to confirm the variable definition:
#!/bin/bash # Description: Search for the definition of '--tv-Steps-error-text-color' in LESS files. # Test: Ensure the variable is defined. Expect: At least one match. rg --type less '--tv-Steps-error-text-color\s*:\s*'
26-26: Verify the existence of the.inject-Steps-vars()mixinEnsure that the mixin
.inject-Steps-vars();is defined and properly imported. Missing mixin definitions can lead to compilation errors during the build process.Run the following script to check if the mixin is defined:
✅ Verification successful
.inject-Steps-vars()mixin is properly defined and imported.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for the definition of 'inject-Steps-vars' mixin in LESS files. # Test: Verify that the mixin is defined. Expect: At least one match. rg --type less '@mixin inject-Steps-vars'Length of output: 490
Script:
#!/bin/bash # Description: Search for the definition of 'inject-Steps-vars' mixin in LESS files with corrected pattern. # Test: Verify that the mixin is defined. Expect: At least one match. rg --type less '@mixin\s+inject-Steps-vars\b'Length of output: 47
Script:
#!/bin/bash # Description: Search for the definition of 'inject-Steps-vars' mixin in LESS files using correct LESS syntax. # Test: Verify that the mixin is defined. Expect: At least one match. rg --type less '\.inject-Steps-vars\s*\(.*\)'Length of output: 165
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
New Features
lineproperty to the steps component for enhanced customization.modeoptions for theadvancedproperty to include both mobile and PC.Bug Fixes
Style
Documentation