refactor(milestone): [milestone] refactor the milestone theme. #2233
Conversation
WalkthroughThe changes in this pull request involve a comprehensive update to the styling and naming conventions of milestone components within the theme package. Key modifications include renaming CSS variables from a Changes
Suggested reviewers
Poem
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 (
|
chenxi-20
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 (6)
packages/theme/src/milestone/old-theme.js (1)
6-8: Consider updatingti-prefixed CSS variables for consistency.While the main property names have been updated from
ti-totv-, there are still CSS variables within the values that use theti-prefix (e.g.,var(--ti-common-font-size-base, 12px)). For complete consistency, you might want to consider updating these as well.Here's an example of how you could update one of these:
- 'tv-Milestone-flag-desc-font-size': 'var(--ti-common-font-size-base, 12px)', + 'tv-Milestone-flag-desc-font-size': 'var(--tv-common-font-size-base, 12px)',However, before making this change, please ensure that these CSS variables are also being updated throughout the entire project to maintain consistency and prevent breaking changes.
Also applies to: 11-12, 15-15, 20-21, 25-29
packages/theme/src/milestone/vars.less (4)
19-19: Avoid hard-coded fallback colors for consistency.The variable
--tv-Milestone-bg-colorincludes a fallback color (#fff). If--tv-color-bg-secondaryis always defined in the theme, it's better to rely on the variable without a hard-coded fallback to ensure consistent theming across different environments.Apply this diff to remove the fallback:
- --tv-Milestone-bg-color: var(--tv-color-bg-secondary, #fff); + --tv-Milestone-bg-color: var(--tv-color-bg-secondary);
25-27: Ensure consistent use of size variables.The variables for icon dimensions mix calculations and direct variable usage:
--tv-Milestone-icon-widthusescalc(var(--tv-size-base, 4px) * 8).--tv-Milestone-icon-heightdirectly usesvar(--tv-size-height-md, 32px).Consider standardizing the approach to improve consistency and readability.
69-69: Maintain consistency in border-radius values.The
--tv-Milestone-flag-border-radiusvariable mixes zero and variable-based values:--tv-Milestone-flag-border-radius: 0 var(--tv-border-radius-lg, 8px) var(--tv-border-radius-lg, 8px) 0;Consider using variables for all corners or explicitly stating when a corner should have no radius for clarity.
Apply this diff for consistency:
- --tv-Milestone-flag-border-radius: 0 var(--tv-border-radius-lg, 8px) var(--tv-border-radius-lg, 8px) 0; + --tv-Milestone-flag-border-radius: var(--tv-border-radius-none, 0) var(--tv-border-radius-lg, 8px) var(--tv-border-radius-lg, 8px) var(--tv-border-radius-none, 0);
86-87: Review the necessity of--tv-Milestone-dot-displayvariable.The variable
--tv-Milestone-dot-displayis set tonone, and a comment indicates whether the dot is displayed. If the dot is always hidden, consider removing this variable and associated code to simplify the stylesheet.packages/theme/src/milestone/index.less (1)
25-25: Consider translating code comments to English for consistency.Using English for code comments ensures consistency and improves readability for all contributors who may not understand Chinese.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/theme/src/milestone/aurora-theme.js (1 hunks)
- packages/theme/src/milestone/index.less (2 hunks)
- packages/theme/src/milestone/old-theme.js (1 hunks)
- packages/theme/src/milestone/vars.less (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/theme/src/milestone/aurora-theme.js
🧰 Additional context used
🔇 Additional comments (6)
packages/theme/src/milestone/old-theme.js (3)
2-29: Consistent naming convention change applied.The prefix for all properties has been updated from
ti-milestone-*totv-Milestone-*. This change has been consistently applied across all properties in thetinyMilestoneOldThemeobject. The new naming convention aligns with the refactoring objectives mentioned in the PR summary.
2-29: Values remain unchanged, ensuring consistency.The values associated with each property have been preserved during the renaming process. This maintains the existing styling and behavior of the milestone theme while improving the naming convention.
2-29: Verify impact of naming convention change.While the naming convention change has been consistently applied within this file, it's crucial to ensure that these changes are reflected throughout the codebase where these theme properties are used. This change could potentially break existing code that relies on the old property names.
To verify the impact and usage of the new property names, please run the following script:
This script will help identify any remaining instances of the old naming convention and confirm the usage of the new naming convention across the codebase.
✅ Verification successful
To get a clearer picture of the impact of the naming convention change, we need to perform a more targeted search. Let's run some additional scripts to focus on the milestone-related properties.
It seems the previous script encountered issues with recognizing Vue files. Let's try an alternative approach using more basic Unix commands to search for the milestone properties.
Based on the search results, we can confirm that the naming convention change from "ti-milestone-" to "tv-Milestone-" has been successfully implemented across the codebase. Here's a summary of our findings:
The naming convention change has been applied consistently.
- No instances of the old "ti-milestone-*" naming convention were found in the codebase.
- The new "tv-Milestone-*" naming convention is present in the theme files, including both aurora-theme.js and old-theme.js.
It's worth noting that while the file named "old-theme.js" uses the new naming convention, this is likely intentional to maintain backwards compatibility while updating the property names.
The search also revealed numerous files related to the milestone component, including Vue components, tests, and documentation. This suggests that the component is widely used and well-tested, which reduces the risk of undetected issues caused by the naming change.
Given these findings, it appears that the naming convention change has been thoroughly implemented, and the risk of breaking existing code due to outdated property names is minimal.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of old and new property names across the codebase # Search for old property names (ti-milestone-*) echo "Searching for old property names (ti-milestone-*):" rg --type vue --type js --type ts 'ti-milestone-' -C 2 echo "\nSearching for new property names (tv-Milestone-*):" rg --type vue --type js --type ts 'tv-Milestone-' -C 2 echo "\nChecking for any remaining 'ti-' prefixes in theme files:" rg --type js 'ti-' packages/theme/src/Length of output: 377560
Script:
#!/bin/bash echo "Searching for old milestone property names (ti-milestone-*):" rg 'ti-milestone-' --type vue --type js --type ts -C 2 echo "\nSearching for new milestone property names (tv-Milestone-*):" rg 'tv-Milestone-' --type vue --type js --type ts -C 2 echo "\nSearching for files that might be using milestone properties:" rg -l 'milestone' --type vue --type js --type tsLength of output: 645
Script:
#!/bin/bash echo "Searching for old milestone property names (ti-milestone-*):" find . -type f \( -name "*.vue" -o -name "*.js" -o -name "*.ts" \) -print0 | xargs -0 grep -n "ti-milestone-" -C 2 echo "\nSearching for new milestone property names (tv-Milestone-*):" find . -type f \( -name "*.vue" -o -name "*.js" -o -name "*.ts" \) -print0 | xargs -0 grep -n "tv-Milestone-" -C 2 echo "\nSearching for files that might be using milestone properties:" find . -type f \( -name "*.vue" -o -name "*.js" -o -name "*.ts" \) -print0 | xargs -0 grep -l "milestone"Length of output: 6556
packages/theme/src/milestone/vars.less (2)
31-31: Verify the scaling factor for icon size.The icon size is set using
calc(var(--tv-icon-size, 16px) * 1.5). Ensure that multiplying by1.5achieves the desired visual effect and remains consistent across different screen sizes and resolutions.
45-45: Confirm the negative top offset is intentional.The variable
--tv-Milestone-line-topis set to-16px, introducing a negative offset. Verify that this positioning is intentional and does not cause alignment issues in the layout.packages/theme/src/milestone/index.less (1)
20-20: Verify that allti-prefixed variables have been updated totv-.To maintain consistency with the new naming convention, ensure that no CSS variables with the
ti-prefix remain in the codebase.Run the following script to verify:
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
examples/sites/demos/pc/app/milestone/flag-before.spec.ts (1)
Line range hint
1-58: Consider enhancing test maintainability and readability.While the changes and overall test structure look good, here are some suggestions to improve the test's maintainability and readability:
Replace magic numbers with named constants. For example:
const MIN_FLAG_HEIGHT = 34; const FLAG_LINE_WIDTH = 1; const FLAG_LINE_HEIGHT = 30;Consider extracting the flag content arrays and color arrays into separate constants at the top of the file for easier maintenance.
The
forloops for checking flag properties are quite similar. Consider creating a helper function to reduce code duplication.Would you like me to provide an example of how to implement these suggestions?
examples/sites/demos/pc/app/milestone/solid-style.spec.ts (2)
26-27: Approved: Consistent style updates for milestone iconsThe changes to the icon styles improve the overall consistency and readability of the milestone component. The shift to white backgrounds with darker text colors, along with the removal of box shadows, creates a cleaner and more modern look.
Consider using CSS custom properties (variables) for the colors to improve maintainability and consistency across the component. For example:
const iconStyles = [ { 'background-color': 'var(--tv-milestone-icon-bg, #fff)', 'color': 'var(--tv-milestone-icon-color, #191919)', 'box-shadow': 'none' }, // ... (apply similar changes to other styles) ]This approach would make it easier to update the color scheme in the future and ensure consistency across the component.
Also applies to: 31-32, 37-37, 40-42, 45-47, 50-52
Line range hint
1-103: Overall feedback: Update test to reflect new design intentionsThe changes in this file primarily focus on updating the visual styles and layout of the milestone component. While the test structure remains largely intact, there are several areas where the test could be improved to better reflect and validate the new design:
- Consider adding tests for accessibility, ensuring that the new color contrasts meet WCAG standards.
- Update the test description to mention the new 'solid' style and its characteristics.
- Add tests for responsive behavior, if applicable, given the changes in node width calculations.
- Consider parameterizing the color values and sizes to make the test more maintainable and less prone to small visual changes.
To improve the maintainability of this test, consider extracting the expected styles into a separate configuration object. This would make it easier to update styles in the future and provide a clear overview of the expected component appearance. For example:
const expectedStyles = { default: { backgroundColor: 'rgb(255, 255, 255)', color: 'rgb(25, 25, 25)', boxShadow: 'none' }, completed: { backgroundColor: 'rgb(25, 25, 25)', color: 'rgb(255, 255, 255)' }, // ... other states }; // Then in the test for (let i = 0; i < 2; i++) { await expect(nodeIcons.nth(i)).toHaveCSS('background-color', expectedStyles.completed.backgroundColor); await expect(nodeIcons.nth(i)).toHaveCSS('color', expectedStyles.completed.color); }This approach would make the test more readable and easier to maintain as the component design evolves.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- examples/sites/demos/pc/app/milestone/basic-usage.spec.ts (3 hunks)
- examples/sites/demos/pc/app/milestone/custom-icon-slot.spec.ts (1 hunks)
- examples/sites/demos/pc/app/milestone/flag-before.spec.ts (3 hunks)
- examples/sites/demos/pc/app/milestone/show-number.spec.ts (4 hunks)
- examples/sites/demos/pc/app/milestone/solid-style.spec.ts (4 hunks)
🧰 Additional context used
🔇 Additional comments (23)
examples/sites/demos/pc/app/milestone/custom-icon-slot.spec.ts (1)
13-13: Confirm design change and update documentation if necessary.The change from '12px' to '24px' for the icon font size aligns with the overall refactoring of the milestone theme. This modification doubles the size of the icons, which could significantly impact the visual appearance of the component.
To ensure this change is intentional and properly documented:
- Please confirm that this size increase is part of the intended design changes.
- If this represents a new design standard, consider updating the component's documentation to reflect the new icon size.
Run the following script to check for any other occurrences of the old font size and to verify if the documentation mentions the icon size:
Please review the results of this script to ensure consistency across the codebase and documentation.
examples/sites/demos/pc/app/milestone/flag-before.spec.ts (2)
15-16: Color changes in flag line arrays look good.The updates to
flagAfterLineColorsandflagBeforeLineColorsarrays reflect a change in the design, replacing 'rgb(94, 124, 224)' with 'rgb(25, 25, 25)' (a very dark gray). The test has been correctly updated to match these changes.To ensure these changes align with the intended design:
- Verify that the color changes match the updated design specifications.
- Consider running visual regression tests to confirm the visual impact of these changes.
29-29: Flag element width increase looks good.The expected width of flag elements has been increased from 58px to 96px. This change is consistently applied in both parts of the test (lines 29 and 51).
To ensure these changes are correct and don't cause any layout issues:
- Verify that the width increase aligns with the updated design specifications.
- Check if this change affects the overall layout or responsiveness of the milestone component.
- Consider running visual regression tests to confirm the layout impact of these changes.
Also applies to: 51-51
examples/sites/demos/pc/app/milestone/solid-style.spec.ts (3)
75-75: Approved: Thinner lines between milestone nodesThe reduction of the nodeLines height from 4px to 1px is consistent with the overall simplification of the component's visual style. This change should result in a more subtle and refined appearance for the connections between milestone nodes.
88-88: Approved with suggestion: Verify visual balance with larger icon sizeThe increase in font size for the first two node icons from 12px to 24px will make the completed milestone icons more prominent. This change can improve visibility and emphasis on completed stages.
Please verify that this significant size increase doesn't negatively impact the overall visual balance of the component. Consider the following:
- Check if the larger icons align well with other elements (e.g., text, lines).
- Ensure that the contrast between completed and non-completed milestones is intentional and not too stark.
- Verify that the larger icons don't overshadow other important information in the component.
It might be helpful to get feedback from the design team on this change.
70-70: Verify the impact of reduced nodeWidth on the milestone layoutThe nodeWidth calculation has been changed to half of its previous value. This significant reduction could affect the overall layout and appearance of the milestone component.
Please clarify the reasoning behind this change and ensure that it doesn't negatively impact the component's layout. Consider running the following verification script:
Additionally, it would be helpful to visually inspect the component with this change to ensure it meets the design requirements.
examples/sites/demos/pc/app/milestone/show-number.spec.ts (7)
26-27: LGTM! Verify visual impact of style changes.The color and box-shadow changes for the first two icons appear to be intentional and consistent. These updates likely reflect a design change in the milestone theme.
To ensure these changes align with the intended design, please verify the visual appearance of the milestone component with these updated styles.
Also applies to: 31-32
37-37: LGTM! Consistent style update across all icons.The removal of the box-shadow property for all icon styles ensures a uniform appearance across the milestone component. This change is consistent with the earlier modifications.
Also applies to: 42-42, 47-47, 52-52
40-41: LGTM! Verify color meanings for different statuses.The background and color properties for the last four icon styles have been updated, likely reflecting different milestone statuses. These changes are consistent with the overall style update.
Please confirm that the new colors accurately represent their intended statuses:
- rgb(217, 217, 217) for 'cancel'
- rgb(245, 34, 45) for 'back'
- rgb(250, 173, 20) for 'end'
Also applies to: 45-46, 50-51
70-70: Verify impact of reduced node width calculation.The nodeWidth calculation has been changed to half of its previous value. This modification will result in smaller node widths, potentially affecting the layout and appearance of the milestone component.
Could you please clarify the rationale behind this change? Additionally, it's important to verify that this modification doesn't negatively impact the component's layout or functionality. Consider running visual regression tests to ensure the change produces the desired outcome.
76-76: Verify visual impact of reduced node line height.The expected height of node lines has been reduced from '4px' to '1px'. This change will result in significantly thinner lines connecting the milestone nodes.
Please ensure that this modification aligns with the intended design update. Consider the following:
- Does this change maintain sufficient visibility of the connecting lines?
- Is this consistent with the overall design language of the application?
- Have you conducted visual regression tests to confirm the desired appearance?
89-89: Verify visual hierarchy with increased icon size.The font size for the icons in the first two nodes has been increased from '12px' to '24px'. This change will result in significantly larger icons for these nodes.
Please consider the following points:
- Does this change maintain a balanced visual hierarchy within the milestone component?
- Is the increased size consistent with the overall design language?
- Have you verified that the larger icons don't overlap or interfere with other elements?
- Consider running visual regression tests to ensure the change produces the desired outcome.
103-103: LGTM! Consistent update for post-click icon size check.The font size check for the first two icons after the button click has been updated to '24px', which is consistent with the earlier modification. This change ensures that the test expectations match the new design after the interaction.
examples/sites/demos/pc/app/milestone/basic-usage.spec.ts (10)
28-29: Updated icon style properties for the first iconThe
colorproperty has been updated torgb(25, 25, 25)and thebox-shadowhas been set tononefor the first icon style. This change aligns with the updated design specifications.
33-34: Updated icon style properties for the second iconSimilarly, the
colorproperty has been updated torgb(25, 25, 25)and thebox-shadowset tononefor the second icon style.
39-39: Removed 'box-shadow' for the third iconThe
box-shadowproperty has been set tononefor the third icon, consistent with the design updates.
42-44: Updated icon style properties for the fourth iconThe
background-coloris changed torgb(255, 255, 255),colortorgb(217, 217, 217), andbox-shadowset tononefor the fourth icon style, matching the new design guidelines.
47-49: Updated icon style properties for the fifth iconThe
background-coloris updated torgb(255, 255, 255),colortorgb(151, 167, 219), andbox-shadowtononefor the fifth icon style, aligning with the updated design specifications.
52-54: Updated icon style properties for the sixth iconThe
background-coloris set torgb(255, 255, 255),colortorgb(250, 173, 20), andbox-shadowtononefor the sixth icon style, in accordance with the design changes.
67-67: Verify the updated calculation ofnodeWidthThe calculation for
nodeWidthhas been changed toMath.floor(totalWidth / nodeCount / 2). Please confirm that this adjustment correctly reflects the intended layout, as it effectively halves the node width compared to dividing solely bynodeCount.
73-73: Updated node line height to1pxThe expected CSS
heightfornodeLineshas been changed from4pxto1px. Ensure this modification aligns with the design specifications for line thickness.
86-86: Updated icon font size to24pxThe
font-sizefor the SVG icons has been updated to24pxfrom12pxto enhance visibility. Verify that this change is consistent with the new design requirements.
91-92: Updated flags' CSS propertiesThe
widthof the flags is now set to96px, and thepaddingis adjusted to4px. Please confirm that these changes are intended and reflect the desired layout adjustments.
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
Bug Fixes
Documentation