refactor(theme): [filter-panel] refactor filter-panel theme vars #2288
Conversation
WalkthroughThe pull request includes comprehensive updates to the CSS styling of the filter box and filter panel components. Key changes involve renaming CSS variables from the Changes
Possibly related PRs
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 (1)
packages/theme/src/filter-panel/vars.less (1)
14-22: Approve CSS variable updates with a minor suggestion.The updates to CSS variable names and values improve consistency and maintainability:
- The prefix change from
--ti-to--tv-is consistent across all variables.- Using other theme variables (e.g.,
--tv-space-md,--tv-shadow-4-down) promotes theme consistency.- Existing comments are maintained, preserving code documentation.
For even better consistency, consider using a theme variable for the padding values as well, instead of the fixed
24px. For example:- --tv-FilterPanel-padding-y: 24px; - --tv-FilterPanel-padding-x: 24px; + --tv-FilterPanel-padding-y: var(--tv-space-lg, 24px); + --tv-FilterPanel-padding-x: var(--tv-space-lg, 24px);This change would allow for easier theme customization while maintaining the current values as fallbacks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/theme/src/filter-box/index.less (3 hunks)
- packages/theme/src/filter-box/vars.less (1 hunks)
- packages/theme/src/filter-panel/index.less (1 hunks)
- packages/theme/src/filter-panel/vars.less (1 hunks)
🧰 Additional context used
🔇 Additional comments (26)
packages/theme/src/filter-panel/vars.less (1)
12-12: Approve method name change and verify usage.The new method name
.inject-FilterPanel-vars()is more descriptive and follows a better naming convention. This improves code readability and maintainability.Please ensure that all calls to this method have been updated throughout the codebase. Run the following script to verify:
✅ Verification successful
| &:hover { | ||
| background: var(--ti-filter-box-btn-hover-bg-color); | ||
| } | ||
|
|
||
| &.disabled { | ||
| cursor: not-allowed; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace hardcoded disabled state colors with variables
The .disabled class and .filter-icon-close use hardcoded color values (e.g., color: #b5bbc1;, fill: #b5bbc1;). Replacing these with theme variables improves maintainability and theming flexibility.
Example changes:
- color: #b5bbc1;
+ color: var(--tv-FilterBox-disabled-text-color);
- fill: #b5bbc1;
+ fill: var(--tv-FilterBox-disabled-icon-color);Define these new variables in vars.less to correspond with the desired disabled state colors.
Also applies to: 109-109
| border-radius: 4px; | ||
| background: var(--ti-filter-box-btn-bg-color); | ||
| background: var(--tv-FilterBox-btn-bg-color); | ||
| color: #161e26; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using CSS variables instead of hardcoded color values
The color #161e26 is hardcoded in the .filter-box and .value classes. For consistency and easier theme customization, consider replacing hardcoded colors with theme variables.
Apply the following changes:
- color: #161e26;
+ color: var(--tv-FilterBox-text-color);Ensure that --tv-FilterBox-text-color is defined in vars.less with the appropriate value.
Also applies to: 94-94
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/theme/src/filter-box/index.less (1)
19-19: LGTM: Method name update improves clarityThe change from
.component-css-vars-filter-box()to.inject-FilterBox-vars()improves clarity and consistency in the codebase. This new naming convention appears to be part of a larger refactoring effort.Consider updating any relevant documentation to reflect this method name change, ensuring that developers are aware of the new naming convention.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- examples/sites/demos/pc/app/filter-panel/basic-usage.spec.ts (1 hunks)
- examples/sites/demos/pc/app/filter-panel/event.spec.ts (1 hunks)
- packages/theme/src/filter-box/index.less (1 hunks)
- packages/theme/src/filter-box/vars.less (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/theme/src/filter-box/vars.less
🧰 Additional context used
🔇 Additional comments (8)
examples/sites/demos/pc/app/filter-panel/event.spec.ts (1)
16-16: 🛠️ Refactor suggestionConsider the trade-offs of the new selector implementation.
The change from
page.getByRole('radio', { name: '等于' }).click()topage.locator('label').filter({ hasText: '等于' }).click()has both advantages and potential drawbacks:Pros:
- May be more reliable if the radio button itself is not directly clickable.
- Could be more specific if there are multiple radio buttons with similar roles.
Cons:
- Less aligned with Playwright's recommendation to use role-based selectors for accessibility testing.
- Potentially more brittle if the label text or component structure changes.
Consider a hybrid approach that maintains accessibility testing while addressing any specific issues:
await page.getByRole('radio', { name: '等于' }) .locator('xpath=./ancestor::label') .click();This suggestion uses the role-based selector to find the radio button, then navigates to its label for clicking. This approach maintains accessibility testing while potentially addressing clickability issues.
To ensure the component structure supports this approach, run the following script:
These tests will help verify the component structure and ensure the suggested approach is viable.
packages/theme/src/filter-box/index.less (7)
36-36: LGTM: Disabled state styling improvedThe update to use
--tv-FilterBox-btn-disabled-text-colorfor the disabled state text color is consistent with the new naming convention and addresses a previous suggestion to use variables instead of hardcoded colors.This change successfully addresses the previous review comment about replacing hardcoded disabled state colors with variables.
Also applies to: 40-40
44-44: LGTM: Disabled icon color now uses a variableThe update to use
--tv-FilterBox-icon-color-disabledfor the disabled icon color is consistent with the new naming convention and improves maintainability.This change successfully addresses the previous review comment about replacing hardcoded colors with variables for the disabled state.
51-53: LGTM: Title styling updated with new variablesThe title styling has been updated to use the new CSS variables, which is consistent with the overall refactoring. The change from
margin-right: 0px;tomargin-right: 0;is a minor but welcome syntax improvement.
103-106: LGTM: Icon styling consistency improvedThe updates to icon size and color variables for both the main icon and the close icon maintain consistency with the new naming convention. The addition of a hover state color for the close icon enhances user interaction feedback.
Also applies to: 115-116, 118-122
61-61: LGTM: Comprehensive update of CSS variablesThe changes to color, size, positioning, and margin variables are consistent with the new naming convention and provide more granular control over the component's styling. This update improves the overall maintainability and customizability of the FilterBox component.
Please ensure that all new variables (e.g.,
--tv-FilterBox-help-btn-margin-right,--tv-FilterBox-help-btn-position-top) are properly defined in the correspondingvars.lessfile. Run the following script to verify:#!/bin/bash # Description: Verify the new CSS variable definitions in vars.less # Test: Search for the new variable definitions rg --type less -e '--tv-FilterBox-btn-active-text-color' -e '--tv-FilterBox-icon-size' -e '--tv-FilterBox-help-btn-margin-right' -e '--tv-FilterBox-icon-color' -e '--tv-FilterBox-help-btn-position-top' -e '--tv-FilterBox-icon-color-hover' packages/theme/src/filter-box/vars.lessAlso applies to: 67-67, 72-77, 81-81
90-90: LGTM: Improved value styling and customizationThe addition of
vertical-align: middle;improves layout consistency. The updates to font size and color variables, including the new placeholder color variable, enhance customization options and maintain consistency with the new naming convention.Please ensure that the new variable
--tv-FilterBox-btn-text-color-placeholderis properly defined in the correspondingvars.lessfile. Run the following script to verify:#!/bin/bash # Description: Verify the new CSS variable definition in vars.less # Test: Search for the new variable definition rg --type less -e '--tv-FilterBox-btn-text-color-placeholder' packages/theme/src/filter-box/vars.lessAlso applies to: 94-95, 98-98
125-129: LGTM: Enhanced hover state behavior and stylingThe update to apply the hover state only when the component is not disabled improves the overall user experience. The introduction of a new variable for the hover background color and the update to the icon color on hover enhance customization options and visual feedback.
Please ensure that the new variable
--tv-FilterBox-btn-hover-bg-coloris properly defined in the correspondingvars.lessfile. Run the following script to verify:✅ Verification successful
Verified: CSS variable
--tv-FilterBox-btn-hover-bg-coloris properly defined invars.less.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new CSS variable definition in vars.less # Test: Search for the new variable definition rg --type less -e '--tv-FilterBox-btn-hover-bg-color' packages/theme/src/filter-box/vars.lessLength of output: 158
| @@ -13,7 +13,7 @@ test('基本用法', async ({ page }) => { | |||
|
|
|||
| await button.click() | |||
| await popPanel.isVisible() | |||
| await page.getByRole('radio', { name: '等于' }).click() | |||
| await page.locator('label').filter({ hasText: '等于' }).click() | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider the trade-offs of the new selector approach
The change from page.getByRole('radio', { name: '等于' }).click() to page.locator('label').filter({ hasText: '等于' }).click() has both advantages and potential drawbacks:
Pros:
- More flexible if the element structure changes
- Might work better with custom-styled radio buttons
Cons:
- Less accessibility-focused
- Potentially less specific if there are multiple elements with the same text
While the new approach works, it's generally recommended to use role-based selectors when possible for better accessibility testing. Consider using a data attribute for more specific targeting:
page.locator('[data-testid="equal-radio"]').click()This approach combines specificity with maintainability. If you decide to keep the current implementation, please ensure it's consistently applied across all similar scenarios in the test suite.
| height: var(--tv-FilterBox-btn-height); | ||
| line-height: var(--tv-FilterBox-btn-height); | ||
| border-radius: 4px; | ||
| background: var(--ti-filter-box-btn-bg-color); | ||
| color: #161e26; | ||
| padding: 4px var(--ti-filter-box-btn-padding-horizontal); | ||
| background: var(--tv-FilterBox-btn-bg-color); | ||
| color: var(--tv-FilterBox-btn-text-color); | ||
| padding: 4px var(--tv-FilterBox-btn-padding-x); |
There was a problem hiding this comment.
💡 Codebase verification
Issues Found: Missing CSS Variable Definitions
The updated CSS variables in index.less (--tv-FilterBox-btn-height, --tv-FilterBox-btn-bg-color, --tv-FilterBox-btn-text-color, --tv-FilterBox-btn-padding-x) are not defined in vars.less. This may lead to styling issues in the FilterBox component.
- Ensure that all updated variables are properly defined in
packages/theme/src/filter-box/vars.less.
🔗 Analysis chain
LGTM: CSS variable naming convention update
The update of CSS variable names from --ti- prefix to --tv- prefix is consistent with the refactoring effort. The new names are more specific to the FilterBox component, improving clarity and maintainability.
Please ensure that these new variables are properly defined in the corresponding vars.less file. Run the following script to verify:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new CSS variable definitions in vars.less
# Test: Search for the new variable definitions
rg --type less -e '--tv-FilterBox-btn-height' -e '--tv-FilterBox-btn-bg-color' -e '--tv-FilterBox-btn-text-color' -e '--tv-FilterBox-btn-padding-x' -e '--tv-FilterBox-btn-font-size' packages/theme/src/filter-box/vars.less
Length of output: 689
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
Refactor