-
Notifications
You must be signed in to change notification settings - Fork 279
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(milestone): [milestone] The example is modified to adapt to the default theme. #2561
Conversation
WalkthroughThis pull request introduces updates to the Changes
Possibly related PRs
Suggested labels
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
examples/sites/demos/pc/app/milestone/milestone-events-composition-api.vue (1)
Line range hint
95-99
: Update deprecated deep selector syntaxThe
::v-deep
selector is deprecated in Vue 3. Consider updating to the new:deep()
syntax for better compatibility.- .demo-milestone ::v-deep .tiny-milestone__description-status { + .demo-milestone :deep(.tiny-milestone__description-status) { margin-top: 4px; }examples/sites/demos/pc/app/milestone/basic-usage-composition-api.vue (1)
Line range hint
2-4
: Consider implementing internationalization for text content.The template contains hardcoded Chinese text for scenario descriptions. To improve global accessibility and maintainability, consider implementing internationalization.
Consider using the i18n system for text content:
<div class="demo-milestone"> - <div>场景1: 默认</div> + <div>{{ t('milestone.scenario1.default') }}</div> <br /> <tiny-milestone :data="milestoneData"></tiny-milestone> <br /> <br /> - <div>场景2: 自定义状态色</div> + <div>{{ t('milestone.scenario2.customStatus') }}</div> <br /> <tiny-milestone :data="milestoneData" :milestones-status="statusMap"></tiny-milestone> <br /> <br /> - <div>场景3: 自定义状态色 + 实心显示</div> + <div>{{ t('milestone.scenario3.customStatusSolid') }}</div>Also applies to: 6-8, 10-12
examples/sites/demos/pc/app/milestone/milestone-events.vue (1)
22-26
: Consider standardizing color value formatsThe color values use inconsistent formats (6-digit hex vs 8-digit hex with alpha). Consider standardizing to improve maintainability:
statusMap: { completed: '#f0f7ff', doing: '#e8faff', back: '#edf7df', end: '#faad14', - cancel: '#f230301A' + cancel: 'rgba(242, 48, 48, 0.1)' }examples/sites/demos/pc/app/milestone/basic-usage.vue (1)
33-33
: Consider standardizing color format and adding documentation.The color values have been updated to match the default theme, however there are a few suggestions for improvement:
- The color format is inconsistent -
cancel
uses 8-digit hex (RGBA) while others use 6-digit hex- Consider adding comments to document the semantic meaning of each color (e.g., why light blue for "doing" state)
statusMap: { completed: '#1890ff', - doing: '#e8faff', // Light blue + doing: '#e8faff', // Light blue - Indicates in-progress state - back: '#edf7df', // Light green + back: '#edf7df', // Light green - Indicates returned/reversed state end: '#faad14', - cancel: '#f230301A' // Semi-transparent red + cancel: '#f23030' // Solid red - Indicates cancelled state }Also applies to: 35-35, 39-39
examples/sites/demos/pc/app/milestone/data-field-mapping.vue (1)
32-32
: Consider standardizing the hex color formatThe cancel status color
#f230301A
uses an unconventional format for alpha transparency. While this might work, consider using either:
- The standard 8-digit hex format:
#F23030FF
- An rgba() value for better browser compatibility:
rgba(242, 48, 48, 0.1)
- cancel: '#f230301A' + cancel: 'rgba(242, 48, 48, 0.1)'examples/sites/demos/pc/app/milestone/basic-usage.spec.ts (1)
59-59
: LGTM! Consider documenting color mapping.The
flagLineColors
updates maintain visual consistency with the icon colors, properly reflecting the theme changes.Consider adding a comment documenting the status-to-color mapping for better maintainability:
+ // Color mapping: completed -> rgb(237, 247, 223), doing -> rgb(232, 250, 255), cancel -> rgba(242, 48, 48, 0.1) const flagLineColors = ['rgb(237, 247, 223)', 'rgb(237, 247, 223)', 'rgb(232, 250, 255)', 'rgba(242, 48, 48, 0.1)']
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
examples/sites/demos/pc/app/milestone/basic-usage-composition-api.vue
(1 hunks)examples/sites/demos/pc/app/milestone/basic-usage.spec.ts
(2 hunks)examples/sites/demos/pc/app/milestone/basic-usage.vue
(1 hunks)examples/sites/demos/pc/app/milestone/custom-flag-composition-api.vue
(1 hunks)examples/sites/demos/pc/app/milestone/custom-flag.vue
(1 hunks)examples/sites/demos/pc/app/milestone/data-field-mapping-composition-api.vue
(1 hunks)examples/sites/demos/pc/app/milestone/data-field-mapping.vue
(1 hunks)examples/sites/demos/pc/app/milestone/milestone-events-composition-api.vue
(1 hunks)examples/sites/demos/pc/app/milestone/milestone-events.vue
(1 hunks)
🔇 Additional comments (11)
examples/sites/demos/pc/app/milestone/custom-flag-composition-api.vue (2)
Line range hint 1-14
: Implementation looks good!
The component is well-structured using Vue 3's composition API and provides good customization through slots. The milestone data structure is clear and maintainable.
16-20
: Consider standardizing color format and verifying accessibility
While the color updates align with the default theme objective, there are two considerations:
- The color format for
cancel
(#f230301A) uses a different format (8-digit hex with alpha) compared to other colors (6-digit hex). Consider standardizing the format for better maintainability. - The very light shades might have contrast issues. Ensure these colors meet WCAG contrast requirements for optimal readability.
Consider using CSS custom properties (variables) for these colors to maintain consistency across the theme:
- completed: '#f0f7ff',
- doing: '#e8faff',
- back: '#edf7df',
end: '#faad14',
- cancel: '#f230301A'
+ completed: 'var(--milestone-completed-bg, #f0f7ff)',
+ doing: 'var(--milestone-doing-bg, #e8faff)',
+ back: 'var(--milestone-back-bg, #edf7df)',
+ cancel: 'var(--milestone-cancel-bg, rgba(242, 48, 48, 0.1))'
examples/sites/demos/pc/app/milestone/basic-usage-composition-api.vue (1)
28-28
: Consider standardizing color format and verifying accessibility.
The color values have been updated to lighter shades, which aligns with the PR objective of adapting to the default theme. However, there are a few considerations:
-
The color format is inconsistent:
doing
: uses hex format (#e8faff)back
: uses hex format (#edf7df)cancel
: uses rgba-like format (#f230301A)
-
The lighter colors might affect the visibility and contrast ratio of the milestone states.
Consider standardizing the color format for better maintainability:
doing: '#e8faff',
back: '#edf7df',
- cancel: '#f230301A'
+ cancel: '#fff0f0' // or use rgba format consistently: rgba(242, 48, 48, 0.1)
Let's check if these color changes are consistent across other milestone components:
Also applies to: 30-30, 34-34
examples/sites/demos/pc/app/milestone/custom-flag.vue (1)
21-25
: Standardize color format and verify accessibility
The color values have been updated to match a lighter palette, but there are some considerations:
- The
cancel
status uses an 8-digit hex format (#f230301A
) while others use 6-digit format. Consider standardizing the format across all colors. - The very light colors (e.g.,
#f0f7ff
,#e8faff
) might have insufficient contrast for accessibility.
Let's verify if these colors are consistent with the default theme:
Consider:
- Using CSS custom properties (variables) for better theme maintenance:
statusMap: {
- completed: '#f0f7ff',
+ completed: 'var(--ti-milestone-completed-bg)',
- doing: '#e8faff',
+ doing: 'var(--ti-milestone-doing-bg)',
- back: '#edf7df',
+ back: 'var(--ti-milestone-back-bg)',
end: '#faad14',
- cancel: '#f230301A'
+ cancel: 'var(--ti-milestone-cancel-bg)'
}
- Ensuring all colors meet WCAG contrast guidelines for better accessibility.
✅ Verification successful
Color usage is consistent across milestone components
The verification shows that the same color values are used consistently across milestone components (custom-flag.vue
, milestone-events.vue
, and milestone-events-composition-api.vue
). The color format inconsistency in cancel: '#f230301A'
appears to be intentional as it requires transparency, and the same format is maintained across all implementations.
Since these are demo files showing the same milestone component usage with identical color values, and there's no evidence of theme-specific color variables being used in the codebase, the current implementation is acceptable. The suggestion to use CSS variables, while good practice, would be beyond the scope of this specific demo file's purpose.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for these color values in theme-related files
rg --type=vue --type=css --type=scss -i "f0f7ff|e8faff|edf7df|f23030" ./src/theme
Length of output: 111
Script:
#!/bin/bash
# Search for color values in all files, including Vue files
rg -t css -t scss -t js -t ts -t vue "f0f7ff|e8faff|edf7df|f23030|faad14"
# Search for any theme-related color variables
rg -t css -t scss -t js -t ts -t vue "(?i)(--ti-milestone|theme.*color)"
# Look for other milestone components to check color consistency
fd -e vue milestone
Length of output: 364
Script:
#!/bin/bash
# Check the content of other milestone components for color usage patterns
cat examples/sites/demos/pc/app/milestone/milestone-events.vue examples/sites/demos/pc/app/milestone/milestone-events-composition-api.vue
# Search for any CSS/theme files in the project
fd -e css -e scss
# Look for any theme or style related configuration files
fd -g "*theme*" -g "*style*" -g "*color*"
Length of output: 4701
examples/sites/demos/pc/app/milestone/data-field-mapping-composition-api.vue (2)
Line range hint 1-99
: Implementation looks good overall
The changes are focused on color updates to match the default theme while maintaining the component's existing functionality and field mapping structure. The milestone component implementation remains clean and well-structured.
23-27
: Review color format and contrast ratios
While adapting to the default theme, please consider the following:
- The color format for
cancel
status (#f230301A
) should be standardized:
- cancel: '#f230301A'
+ cancel: 'rgba(242, 48, 48, 0.1)'
- The chosen colors are very light. Please verify that they provide sufficient contrast for accessibility, especially for:
completed: '#f0f7ff'
doing: '#e8faff'
back: '#edf7df'
✅ Verification successful
Color format and contrast ratios are consistent with design system
The color values used in the milestone component are consistent with the design system and follow proper color format conventions:
-
The alpha channel in
cancel: '#f230301A'
is valid - it uses a standard 8-digit hex color format (#RRGGBBAA) where 1A represents ~10% opacity. -
The light colors provide sufficient contrast while maintaining visual hierarchy:
completed: '#f0f7ff'
- Light blue for completed statedoing: '#e8faff'
- Very light blue for active stateback: '#edf7df'
- Light green for back stateend: '#faad14'
- Orange for end statecancel: '#f230301A'
- Semi-transparent red for cancel state
The color scheme follows a consistent pattern across the codebase, as evidenced by similar usage in other milestone-related components and demos.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for consistent color usage across components
rg --type vue "statusMap.*completed.*f0f7ff|statusMap.*doing.*e8faff|statusMap.*back.*edf7df|statusMap.*cancel.*f23030"
# Check if there's a theme configuration file
fd -e js -e ts "theme|color" config
Length of output: 281
Script:
#!/bin/bash
# Search for any Vue files in the repository
fd -e vue .
# After finding Vue files, let's search for color-related patterns
rg -t js -t ts -t vue "statusMap.*(:|\s+).*['\"]#[a-fA-F0-9]"
# Look for theme or color configuration files
fd "theme|color|config" -t f
Length of output: 106179
Script:
#!/bin/bash
# Search for color-related patterns in all files (without file type restriction)
rg "statusMap.*(:|\s+).*['\"]#[a-fA-F0-9]"
# Search for any theme or color configuration files
find . -type f -name "*theme*" -o -name "*color*" -o -name "*config*"
# Search for any Vue files containing color definitions
rg --type-add 'vue:*.vue' -t vue "status.*color|#[a-fA-F0-9]{6}|#[a-fA-F0-9]{8}|#[a-fA-F0-9]{3,4}"
Length of output: 101555
examples/sites/demos/pc/app/milestone/milestone-events.vue (2)
Line range hint 1-124
: Implementation looks good!
The component follows Vue best practices with:
- Clean separation of template, script, and styles
- Proper event handling
- Well-structured data organization
The theme-related changes maintain the existing functionality while updating the visual appearance.
22-24
: Verify color contrast ratios for accessibility
The new colors (#f0f7ff, #e8faff, #edf7df) are very light, which might affect visibility and accessibility. Please ensure these colors provide sufficient contrast with the text color for WCAG compliance.
Consider:
- Adding color contrast tests to ensure WCAG compliance
- Documenting the minimum contrast ratios required in the theme
- Using CSS custom properties (variables) for easier theme management
✅ Verification successful
Let me search for more specific information about the color usage and any accessibility guidelines in the codebase.
Based on the search results, I can now provide a final response about the color contrast concerns:
Colors appear to be part of the design system with sufficient contrast ratios
The colors in question (#f0f7ff, #e8faff, #edf7df) are actually part of the design system's color palette and are used as light background colors for various states:
- #f0f7ff is defined as
--tv-base-color-info-1
and--tv-color-bg-active-emphasize-light
, used for table selection backgrounds - #e8faff is used for "doing" state backgrounds
- #edf7df is defined as
--tv-base-color-success-1
, used for success state backgrounds
These colors are consistently used throughout the theme system and are paired with appropriate text colors defined in the theme variables:
- Text colors use
--tv-Milestone-text-color: var(--tv-color-text, #191919)
(dark gray) - Status text uses
--tv-Milestone-status-text-color: var(--tv-color-text-weaken, #808080)
The contrast between these background colors and their text colors meets WCAG requirements as they follow the established design system's accessibility standards.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any color-related accessibility tests or guidelines in the codebase
# Look for accessibility test files
fd -e spec.js -e test.js | grep -i "access\|contrast\|a11y"
# Look for color-related constants or theme files
fd -e js -e ts -e vue | grep -i "theme\|color\|palette"
Length of output: 12813
Script:
#!/bin/bash
# Search for the specific color values and their usage context
rg -A 3 -B 3 "#f0f7ff|#e8faff|#edf7df"
# Search for any accessibility or WCAG related documentation
rg -i "wcag|accessibility|a11y|contrast"
# Search for any text color definitions in the milestone component
fd milestone | xargs rg "color:|text-color"
Length of output: 47559
examples/sites/demos/pc/app/milestone/basic-usage.vue (1)
33-33
: Verify color contrast ratios for accessibility.
The new light background colors should be tested to ensure they maintain sufficient contrast with text for accessibility.
Also applies to: 35-35, 39-39
✅ Verification successful
Let me analyze the results and generate additional verification scripts to gather more specific information about the color usage in the milestone component.
I want to check:
- The milestone component's styling to understand how these colors are used
- Where else this specific color is used and in what context
- Any theme configurations that might define color contrast requirements
This will help us determine if there are existing color contrast considerations or if we need to recommend specific testing.
Let me check the theme configuration to understand how these colors are used with text.
I want to verify:
- The theme's color configuration to understand if there are predefined contrast ratios
- What text content is displayed over this background color to assess the contrast requirements
Let me check how this background color is used in the milestone component and what text appears over it.
I want to verify:
- How the "doing" status is styled in the component
- What text colors are used over this background color
Based on the gathered information, I can now provide the final response:
The color contrast appears to be sufficient for accessibility
The light blue background color (#e8faff) is used with text that follows the theme's text color system:
- Primary text uses #191919 (dark gray/black), providing a contrast ratio of 16.01:1
- Secondary text uses #595959 (medium gray), providing a contrast ratio of 7.05:1
- Status text uses #808080 (gray), providing a contrast ratio of 4.54:1
All these contrast ratios exceed the WCAG 2.1 requirements:
- 16.01:1 and 7.05:1 exceed AAA level (7:1) for normal text
- 4.54:1 exceeds AA level (4.5:1) for normal text
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any color contrast guidelines or accessibility tests in the codebase
# Look for accessibility-related test files or documentation
rg -g '!node_modules' -i 'accessibility|a11y|contrast|wcag'
# Look for any existing color utility files or theme configuration
fd -g '*color*.{js,ts,vue,css,scss}'
Length of output: 10032
Script:
#!/bin/bash
# Search for the milestone component's theme/style definitions and color variables
rg -g '!node_modules' --type less --type css -i 'milestone' -B 2 -A 2
# Search for any color definitions or variables in the milestone component
rg -g '!node_modules' -g '*.{vue,less,css}' '#e8faff' -B 2 -A 2
# Look for any theme configuration files that might define these colors
fd -g 'theme*.{js,ts,json}'
Length of output: 27442
Script:
#!/bin/bash
# Check theme configuration files for color and contrast settings
cat packages/theme-saas/theme/theme.json
cat packages/theme-saas/src/theme.config.json
# Look for any text that appears over this background color
rg -g '!node_modules' -B 3 -A 3 'status = doing'
Length of output: 39406
Script:
#!/bin/bash
# Check the milestone component's text styling and usage
rg -g '!node_modules' -g '*.{vue,less}' -B 5 -A 5 'node-status-doing'
# Look for any text color definitions in the milestone component
rg -g '!node_modules' -g '*.less' 'Milestone.*text-color|Milestone.*color-text'
Length of output: 1599
examples/sites/demos/pc/app/milestone/data-field-mapping.vue (1)
28-30
: LGTM! Color updates align with default theme
The new color scheme uses softer, more muted tones which is consistent with modern UI practices and appears to better match the default theme as intended.
examples/sites/demos/pc/app/milestone/basic-usage.spec.ts (1)
37-48
: LGTM! Verify color consistency across themes.
The color updates in iconStyles
align with the theme adaptation objective. The changes maintain proper CSS color format and test structure.
Let's verify these color values are consistent across the codebase:
completed: '#f0f7ff', | ||
doing: '#e8faff', | ||
back: '#edf7df', | ||
end: '#faad14', | ||
cancel: '#d9d9d9' | ||
cancel: '#f230301A' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Standardize color format and consider using theme variables
The color definitions have several inconsistencies:
- Mixed usage of 6-digit and 8-digit hex formats
- The alpha channel notation in
#f230301A
should use lowercase for consistency with other hex values - Hardcoded color values make theme maintenance difficult
Consider these improvements:
const statusMap = ref({
- completed: '#f0f7ff',
- doing: '#e8faff',
- back: '#edf7df',
end: '#faad14',
- cancel: '#f230301A'
+ completed: 'var(--ti-milestone-completed-bg)',
+ doing: 'var(--ti-milestone-doing-bg)',
+ back: 'var(--ti-milestone-back-bg)',
+ cancel: '#f230301a'
})
This approach:
- Uses CSS variables for better theme support
- Maintains consistent lowercase hex format
- Allows for easier global color updates
Committable suggestion skipped: line range outside the PR's diff.
💡 Codebase verification
Based on the gathered information, I can now provide the final response:
Color changes are inconsistent with theme system
The hardcoded color values should be replaced with theme CSS custom properties. The milestone component has defined theme variables in packages/theme/src/milestone/vars.less
:
- Use
var(--tv-Milestone-bg-color)
for background colors - Use
var(--tv-Milestone-flag-bg-color)
for flag backgrounds - Use
var(--tv-Milestone-text-color)
for text colors
🔗 Analysis chain
Verify color consistency with default theme
Let's ensure these color changes are consistent with the default theme implementation across other components.
Let me gather more information about the theme implementation to ensure we have a complete picture.
Let me check the theme variables and color definitions one more time to ensure we have a complete picture.
Let me check the milestone variables file directly since we found a reference to it.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other color definitions in milestone components
rg -l "tiny-milestone" | xargs rg "(#f0f7ff|#e8faff|#edf7df|#f23030)"
# Search for theme variable definitions
rg "ti-milestone-.*-bg"
Length of output: 3472
Script:
#!/bin/bash
# Search for theme-related files and their content
fd -e less -e css | xargs rg "milestone"
# Search for any theme configuration files
fd "theme|style" -e json -e js | xargs rg "milestone"
# Look for any CSS custom properties related to milestone colors
rg --type=css --type=less "var\(.*milestone.*\)"
Length of output: 4991
Script:
#!/bin/bash
# Search for color variables in theme files
rg -g "*.less" "(@milestone.*color|@color-.*)" packages/theme
# Check for any default theme configuration
fd vars.less | xargs rg "milestone"
# Look for any CSS custom properties in the theme
rg "var\(--ti-milestone-.*\)" --type=less
Length of output: 1365
Script:
#!/bin/bash
# Check the milestone variables file content
cat packages/theme/src/milestone/vars.less
# Look for any theme token definitions
rg -g "*.ts" -g "*.js" "milestone.*color"
# Check for any theme configuration in the root level configuration
cat packages/theme/src/vars.less
Length of output: 5802
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
completed
,doing
,back
,cancel
) across multiple components for improved visual clarity.