feat(alert): [alert] Adapting to the SMB theme #2169
Conversation
WalkthroughThe changes in this pull request primarily focus on updates to the alert component's styling and structure. This includes modifications to CSS properties in test files, adjustments to the alert component's size, and enhancements to the corresponding LESS files. Additionally, a new constant for a "simple" alert type has been introduced in the Vue component, expanding the alert's categorization options. Changes
Possibly related PRs
Suggested labels
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (3)
examples/sites/demos/pc/app/alert/basic-usage.spec.ts (1)
Line range hint
1-41: Consider enhancing test maintainability and readability.The test structure is sound and covers essential aspects of the alert component. However, consider the following suggestions to improve maintainability and readability:
Extract color values into constants at the top of the file. This would make it easier to update colors in the future and improve readability.
Consider using a loop or a custom function to test the colors for all alert types. This would reduce code duplication and make it easier to add or modify alert types in the future.
Add comments to clearly separate different sections of the test (e.g., color testing, visibility testing, functionality testing).
Here's an example of how you could implement suggestions 1 and 2:
const ALERT_COLORS = { info: 'rgb(222, 236, 255)', error: 'rgb(252, 227, 225)', success: 'rgb(230, 242, 213)', warning: 'rgb(255, 235, 209)' }; // ... existing imports and test setup ... // Test colors for all alert types Object.entries(ALERT_COLORS).forEach(([type, color], index) => { await expect(alertBox.nth(index)).toHaveCSS('background-color', color); await expect(alertBox.nth(index)).toHaveCSS('border-bottom-color', color); }); // ... rest of the test ...These changes would make the test more maintainable and easier to update in the future.
packages/theme/src/alert/index.less (1)
152-152: LGTM: Enhanced centering for alerts withis-centerclassThe addition of
align-items: center;to the.is-centerclass improves the centering of alert contents. This change, combined with the existingjustify-content: center;, ensures both horizontal and vertical centering, which is an improvement over the previoustext-align: center;approach.Consider adding
text-align: center;back to ensure inline content within the alert is also centered:.is-center { justify-content: center; align-items: center; + text-align: center; }This will ensure that any inline text or elements are also centered horizontally within the alert.
packages/theme/src/alert/vars.less (1)
Line range hint
1-159: Summary of Alert Component Theme ChangesThe changes to the alert component's theme variables appear to be in line with adapting to the SMB theme, as mentioned in the PR objectives. However, there are a few points that need attention:
- The change to
--ti-alert-large-icon-font-sizeshould be verified to ensure it meets the design specifications for the SMB theme.- The use of a hard-coded color value for
--ti-alert-simple-icon-colormay reduce theme flexibility. Consider using a theme variable instead.- The removal of margin-related variables needs to be verified to ensure it doesn't negatively impact the component's layout and responsiveness.
Please address these points and provide any necessary visual confirmations to ensure the alert component meets the SMB theme requirements while maintaining flexibility and consistency.
When adapting components to new themes, it's generally better to introduce new theme-specific variables rather than modifying existing ones or using hard-coded values. This approach allows for better maintainability and easier theme switching in the future.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- examples/sites/demos/pc/app/alert/basic-usage.spec.ts (1 hunks)
- examples/sites/demos/pc/app/alert/size.spec.ts (1 hunks)
- packages/theme/src/alert/index.less (3 hunks)
- packages/theme/src/alert/vars.less (2 hunks)
- packages/vue/src/alert/src/index.ts (1 hunks)
Additional comments not posted (6)
examples/sites/demos/pc/app/alert/size.spec.ts (1)
9-9: Approved: Height adjustment for large alertThe change from 68px to 77px for the large alert's height aligns with the PR objective of adapting to the SMB theme.
Consider adding a comment explaining the reason for this height change, which could be helpful for future maintenance. For example:
// Height increased to 77px to accommodate new SMB theme requirements await expect(largeAlert).toHaveCSS('height', '77px')Please verify if this height change affects other components or layouts that might be using the large alert. You may want to check:
- Any parent components that contain the alert
- Responsive designs that might be affected by this height increase
- Other test cases that might need updating
To help with this verification, you can run the following script:
This will help identify other places in the code that might need adjustment due to this change.
Verification successful
Verified: Height adjustment for large alert
The change from 68px to 77px for the large alert's height appears to be isolated to the test files, and there are no other occurrences that might affect other components or layouts.
It is recommended to add a comment explaining the reason for this height change for future maintenance. For example:
// Height increased to 77px to accommodate new SMB theme requirements await expect(largeAlert).toHaveCSS('height', '77px')Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of the alert height in the codebase rg --type typescript --type vue "tiny-alert--large" rg --type typescript --type vue "68px"Length of output: 160
Script:
#!/bin/bash # Search for other occurrences of the alert height in the codebase rg --type ts --type vue "tiny-alert--large" rg --type ts --type vue "68px"Length of output: 130
Script:
#!/bin/bash # Search for other occurrences of the alert height in the codebase rg "tiny-alert--large" --glob "*.ts" --glob "*.vue" rg "68px" --glob "*.ts" --glob "*.vue"Length of output: 698
examples/sites/demos/pc/app/alert/basic-usage.spec.ts (1)
11-18: Color updates for alert boxes align with SMB theme adaptation.The changes to the background and border colors for the alert boxes are consistent with the PR objective of adapting to the SMB theme. The new colors appear to be more muted and cohesive, which likely improves the overall visual design.
Some observations:
- The background and border-bottom colors are now the same for each alert type, creating a more unified look.
- The color changes are applied consistently across all four alert types (info, error, success, and warning).
- The color differences between alert types are still distinct, maintaining their separate visual identities.
To ensure these color changes are consistent with the design system, let's check if these colors are defined in a central theme file:
packages/theme/src/alert/index.less (2)
34-34: LGTM: Improved vertical alignment for normal alertsThe addition of
align-items: center;to the normal-sized alert improves the vertical alignment of its contents. This change is consistent with the PR objective of adapting to the SMB theme and enhances the overall appearance of the alert component.
192-192: LGTM: Improved title styling with specific line-heightThe addition of
line-height: 20px;to the alert title improves the consistency and potentially the readability of the text. This change contributes to a more polished appearance of the alert component.Please confirm that the
20pxline-height aligns with the SMB theme design system. If there's a design token or variable for title line-heights, consider using that instead of a hard-coded value:Verification successful
Verified:
20pxline-height aligns with the SMB theme design systemThe
line-height: 20px;inpackages/theme/src/alert/index.lessconforms to the design system's specifications. No further changes are required.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for design tokens or variables related to line-height in the theme rg --type less "line-height.*20px" packages/theme/ rg --type less "ti-.*-line-height.*20px" packages/theme/Length of output: 1994
packages/theme/src/alert/vars.less (2)
90-90: LGTM. Verify the intended icon size.The change from
--ti-common-font-size-5to--ti-common-font-size-4for the large icon font size looks good. This adjustment might align better with the SMB theme mentioned in the PR objectives.Please confirm that this change achieves the desired icon size for large alerts in the SMB theme. You may want to visually inspect the rendered component to ensure it meets the design specifications.
Line range hint
1-159: Verify layout after removal of margin variables.The variables
--ti-alert-icon-margin-topand--ti-alert-title-margin-tophave been removed. While this may simplify the component's styling, it's important to ensure that the alert's layout remains correct and consistent across different sizes and types.Please confirm that:
- The alert icon and title are still properly aligned without these margin variables.
- The layout looks correct in all alert types (success, warning, error, info, simple).
- The component remains responsive and maintains its appearance across different screen sizes.
Consider providing before and after screenshots to demonstrate that the layout has not been negatively impacted by this change.
| warning: 'icon-warning', | ||
| simple: 'icon-help' |
There was a problem hiding this comment.
Tip
Codebase Verification
Incomplete implementation of the new 'simple' alert type
The addition of the 'simple' alert type in the ICON_MAP is not fully supported across the codebase. The TITLE_MAP and type prop definitions do not include 'simple', and no other references to the 'simple' type were found. Please ensure that the 'simple' type is consistently implemented throughout the codebase to avoid potential inconsistencies and user confusion.
Analysis chain
Consider refining the implementation of the new 'simple' alert type
The addition of a 'simple' alert type aligns with the PR objective of adapting to the SMB theme. However, there are a few points to consider:
-
The 'simple' type uses the same icon as 'info' (
icon-help). This might lead to confusion for users. Consider using a distinct icon for 'simple' to differentiate it from 'info'. -
The
TITLE_MAPobject hasn't been updated to include a title for the 'simple' type. This might cause issues when trying to display a title for a 'simple' alert. -
The
typeprop definition inalertPropsstill defaults to 'info' and doesn't mention 'simple' as a possible value. This should be updated to include the new 'simple' type.
To address these points, consider the following changes:
- Use a distinct icon for 'simple':
ICON_MAP: {
success: 'icon-success',
error: 'icon-error',
info: 'icon-help',
warning: 'icon-warning',
- simple: 'icon-help'
+ simple: 'icon-simple' // Replace with an appropriate icon
},- Add a title for 'simple' in
TITLE_MAP:
TITLE_MAP: {
success: 'ui.alert.success',
error: 'ui.alert.error',
info: 'ui.alert.info',
- warning: 'ui.alert.warning'
+ warning: 'ui.alert.warning',
+ simple: 'ui.alert.simple'
},- Update the
typeprop definition:
type: {
type: String,
- default: 'info'
+ default: 'info',
+ validator: (value: string) => ['success', 'error', 'info', 'warning', 'simple'].includes(value)
},To ensure full compatibility with the new 'simple' type, run the following script to check for any other occurrences of alert types that might need updating:
This will help identify any other locations where the alert types are used or checked, ensuring comprehensive implementation of the new 'simple' type.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for alert type references in the codebase
echo "Searching for alert type references:"
rg --type typescript --type vue "type.*(?:success|error|info|warning)" -g '!**/index.ts'
Length of output: 208
Script:
#!/bin/bash
# Description: Search for alert type references in the codebase
echo "Searching for alert type references:"
rg --type ts --type tsx --type vue "type.*(?:success|error|info|warning)" -g '!**/index.ts'
Length of output: 204
Script:
#!/bin/bash
# Description: Search for alert type references in the codebase
echo "Searching for alert type references:"
rg --type typescript --type typescriptreact --type vue "type.*(?:success|error|info|warning)" -g '!**/index.ts'
Length of output: 231
Script:
#!/bin/bash
# Description: Search for alert type references in the codebase using file extensions
echo "Searching for alert type references:"
rg "type.*(?:success|error|info|warning)" -g "*.ts" -g "*.tsx" -g "*.vue" -g "!**/index.ts"
Length of output: 70336
| @@ -150,7 +146,7 @@ | |||
| // simple类型边框色 | |||
| --ti-alert-simple-border-color: var(--ti-common-color-info-border, #d3d4d6); | |||
| // simple类型图标色 | |||
| --ti-alert-simple-icon-color: var(--ti-common-color-text-secondary, #575d6c); | |||
| --ti-alert-simple-icon-color: #808080; | |||
There was a problem hiding this comment.
Consider using a theme variable instead of a hard-coded color.
The change from var(--ti-common-color-text-secondary, #575d6c) to #808080 for the simple icon color reduces theme flexibility. While this may align with the SMB theme requirements, it could lead to inconsistencies if the theme is updated in the future.
Consider one of the following alternatives:
- Use an existing theme variable that matches the desired color.
- Create a new theme variable for this specific use case.
For example:
--ti-alert-simple-icon-color: var(--ti-theme-simple-icon-color, #808080);This approach maintains flexibility while still achieving the desired color for the SMB theme.
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