feat(modal): [modal] modify smb theme #2146
Conversation
WalkthroughThe changes in this pull request focus on modifying the modal component's functionality and styling. Key adjustments include enabling the display of the modal header, updating related test cases, and enhancing the CSS styling for modal text and variables. These alterations collectively improve the user interface and visual coherence of the modal, ensuring a more consistent user experience. 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: 1
Outside diff range and nitpick comments (2)
examples/sites/demos/pc/app/modal/show-header-footer.vue (1)
14-14: Consider externalizing UI text for localization supportThe message
'控制显示头部及底部'is hardcoded in the code. For better maintainability and internationalization, consider using a localization mechanism to manage UI text, which will facilitate translations and support for multiple languages.packages/theme/src/modal/index.less (1)
Line range hint
385-409: Remove duplicate@keyframesdefinitions.The
@keyframes modal-fade-inandmodal-fade-outare defined twice at the end of the file. This duplication is unnecessary and could lead to confusion or maintenance issues. Please remove the duplicate definitions to keep the code clean and efficient.Apply this diff to eliminate the duplicates:
-@keyframes modal-fade-in { - 0% { - transform: translate3d(0, -20px, 0); - opacity: 0; - } - - 100% { - transform: translate3d(0, 0, 0); - opacity: 1; - } -} - -@keyframes modal-fade-out { - 0% { - transform: translate3d(0, 0, 0); - opacity: 1; - } - - 100% { - transform: translate3d(0, -20px, 0); - opacity: 0; - } -}
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- examples/sites/demos/pc/app/modal/show-header-footer-composition-api.vue (1 hunks)
- examples/sites/demos/pc/app/modal/show-header-footer.spec.ts (1 hunks)
- examples/sites/demos/pc/app/modal/show-header-footer.vue (1 hunks)
- packages/theme/src/modal/index.less (1 hunks)
- packages/theme/src/modal/vars.less (3 hunks)
Additional comments not posted (7)
examples/sites/demos/pc/app/modal/show-header-footer-composition-api.vue (1)
9-9: Confirm change aligns with SMB theme modificationsChanging
showHeadertotrueensures that both the header and footer are displayed in the modal, which appears to align with the modifications to the SMB theme as outlined in the PR objectives.examples/sites/demos/pc/app/modal/show-header-footer.vue (1)
14-14: Confirm thatshowHeader: truealigns with design expectationsBy setting
showHeader: true, the modal will now display the header, altering the previous behavior where the header was hidden. Please verify that this change aligns with the intended SMB theme modifications and meets the design specifications.examples/sites/demos/pc/app/modal/show-header-footer.spec.ts (1)
9-9: Confirm Intentional Change in Modal Header VisibilityThe test now expects the modal header to be visible after clicking the button, which alters the previous behavior where the header was expected to be not visible. Please verify that this change aligns with the intended modification of the modal component's functionality and update relevant documentation or other tests if necessary.
Run the following script to check for other test cases that may need updates to remain consistent with this change:
packages/theme/src/modal/vars.less (3)
52-53: Addition of new variable--ti-modal-body-message-font-colorapprovedThe new variable
--ti-modal-body-message-font-colorfollows the existing naming conventions and enhances customization options for the message modal content.
80-80: Adjustment of right padding is appropriateUpdating
--ti-modal-message-padding-righttovar(--ti-common-space-4x)aligns the right padding with the intended design specifications.
82-82: Simplification of vertical padding valueChanging
--ti-modal-message-padding-verticalfrom a calculated value tovar(--ti-common-space-3x)simplifies the code and maintains consistency with spacing variables. Please ensure this alteration preserves the desired vertical spacing in the modal layout.packages/theme/src/modal/index.less (1)
192-196: LGTM!The addition of the
.@{modal-prefix-cls}__textclass within.@{modal-prefix-cls}__contentis well-implemented. The styles appropriately utilize the design tokens and maintain consistency with the existing codebase.
| @@ -123,5 +125,5 @@ | |||
| // 头部底线边框设置(hide) | |||
| --ti-modal-header-border-bottom: none; | |||
| // 关闭按钮颜色 | |||
| --ti-modal-btn-close-color: var(--ti-modal-close-btn-icon-color); | |||
| --ti-modal-btn-close-color: var(--ti-common-color-icon-normal); | |||
There was a problem hiding this comment.
Tip
Codebase Verification
Variable --ti-modal-close-btn-icon-color is still present and used in the codebase
The variable --ti-modal-close-btn-icon-color is still defined in packages/theme/src/modal/vars.less and used in packages/theme/src/modal/index.less. This conflicts with the change made to --ti-modal-btn-close-color. To resolve this:
- Update the usage in
packages/theme/src/modal/index.lessto use--ti-modal-btn-close-colorinstead. - Remove the definition of
--ti-modal-close-btn-icon-colorfrompackages/theme/src/modal/vars.less.
These changes will ensure consistency and remove the unused variable.
Analysis chain
Verify usage of --ti-modal-close-btn-icon-color
The variable --ti-modal-btn-close-color now references var(--ti-common-color-icon-normal) instead of var(--ti-modal-close-btn-icon-color). Ensure that --ti-modal-close-btn-icon-color is not used elsewhere in the codebase to prevent inconsistencies or unused variables.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usage of `--ti-modal-close-btn-icon-color` in the codebase.
# Test: Search for occurrences of the variable. Expect: No matches if it's unused.
rg --type less $'--ti-modal-close-btn-icon-color'
Length of output: 482
Script:
#!/bin/bash
# Description: Check for usage of `--ti-modal-close-btn-icon-color` in the codebase.
# Search for the exact variable name
echo "Searching for exact variable name:"
rg -i --type less '--ti-modal-close-btn-icon-color'
# Search for the variable name without leading dashes
echo "Searching for variable name without leading dashes:"
rg -i --type less 'ti-modal-close-btn-icon-color'
Length of output: 917
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
Style