refactor(search): [search] refactor search theme #2186
Conversation
WalkthroughThe changes involve updates to CSS properties and variable naming conventions across several test files and LESS files related to the search component. Specifically, the expected values for certain CSS properties have been modified in test cases, and CSS variables have been renamed to follow a new naming convention. Additionally, a new method for injecting CSS variables has been introduced, reflecting a comprehensive restructuring of the styling approach. Changes
Possibly related PRs
Suggested labels
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: 6
🧹 Outside diff range and nitpick comments (10)
examples/sites/demos/pc/app/search/slot-prefix.spec.ts (1)
Line range hint
1-12: Consider enhancing the test coverage for the search component.While the current test checks for visibility and font size, consider expanding it to provide more comprehensive coverage:
- Verify the SVG content or structure of the prefix element.
- Check additional CSS properties that are crucial for the prefix's appearance (e.g., color, position).
- Add tests for the functionality of the search component, not just its appearance.
Here's an example of how you might expand the test:
test('search component prefix and functionality', async ({ page }) => { page.on('pageerror', (exception) => expect(exception).toBeNull()) await page.goto('search#slot-prefix') const search = page.locator('.tiny-search').first() const prefix = search.locator('.tiny-search__prefix > svg') const input = search.locator('input') // Check prefix visibility and style await expect(prefix).toBeVisible() await expect(prefix).toHaveCSS('font-size', '16px') await expect(prefix).toHaveCSS('color', 'expectedColor') // Check SVG structure (adjust the selector as needed) expect(await prefix.evaluate(el => el.innerHTML)).toContain('<path') // Test search functionality await input.fill('test search') await page.keyboard.press('Enter') // Add expectations for search results or behavior })This expanded test provides more thorough coverage of both the appearance and functionality of the search component.
examples/sites/demos/pc/app/search/mini-mode.spec.ts (1)
26-27: LGTM. Consider refactoring for DRY principle.The changes here are consistent with the earlier modifications and correctly verify that the search component returns to its initial dimensions after being expanded and collapsed. This is a good test for the component's behavior.
To improve the test's maintainability, consider refactoring these repeated expectations into a reusable function. This would adhere to the DRY (Don't Repeat Yourself) principle. Here's a suggested refactor:
async function expectMiniModeCollapsedDimensions(line: Locator) { await expect(line).toHaveCSS('width', '32px') await expect(line).toHaveCSS('border-radius', '32px') } // Then use it in the test: await expectMiniModeCollapsedDimensions(line) // ... (other test steps) await expectMiniModeCollapsedDimensions(line)This refactoring would make future updates easier and reduce the risk of inconsistencies.
examples/sites/demos/pc/app/search/transparent-mode.spec.ts (2)
18-19: LGTM! Consider adding a comment explaining the design change.The update to the expected CSS properties (width and border-radius) from 28px to 32px looks good. This change is consistent with the PR objective of refactoring the search theme.
Consider adding a brief comment explaining the reason for this design change, e.g.:
// Updated mini mode dimensions to 32px for improved visual consistency await expect(line).toHaveCSS('width', '32px') await expect(line).toHaveCSS('border-radius', '32px')This would help future developers understand the rationale behind these specific values.
32-33: LGTM! Consider refactoring for DRY principle.The update to the expected CSS properties (width and border-radius) from 28px to 32px after collapsing the search interface is correct and consistent with the earlier change.
To improve the test's maintainability, consider refactoring these repeated expectations into a reusable function:
const assertLineProperties = async (line: Locator) => { await expect(line).toHaveCSS('width', '32px') await expect(line).toHaveCSS('border-radius', '32px') } // Usage: await assertLineProperties(line)This would allow you to easily update the expected values in one place if they change in the future, adhering to the DRY (Don't Repeat Yourself) principle.
packages/theme/src/search/vars.less (2)
13-13: Approve function name change with a minor suggestion.The new function name
.inject-Search-vars()is more descriptive and follows a consistent naming convention. It clearly indicates the purpose of injecting Search-related variables.Consider using full camelCase for consistency:
.injectSearchVars(). This would align better with common JavaScript/TypeScript naming conventions, which could be beneficial if this LESS file is processed or referenced in JS/TS code.
13-69: Approve consistency in naming and values, suggest adding English translations.The file demonstrates excellent consistency in variable naming and value assignment. The use of the
--tv-Search-prefix and consistent referencing of other--tv-variables enhances maintainability and coherence within the theming system.Consider adding English translations alongside the Chinese comments to improve accessibility for non-Chinese speaking developers. This could be done using a dual-language comment format, for example:
// 搜索框字号 | Search input font size --tv-Search-font-size: var(--tv-font-size-md);To assist with this, you can use the following script to extract all comments for translation:
#!/bin/bash # Description: Extract Chinese comments for translation echo "Chinese comments to be translated:" rg --type less '^\s*//\s*[\p{Han}]' packages/theme/src/search/vars.lessThis will list all Chinese comments in the file, making it easier to add English translations systematically.
packages/theme/src/search/index.less (4)
Line range hint
103-113: Approve the new input button styling with suggestions for improvement.The addition of this CSS rule for the input button's close icon, including the vertical divider, is a good improvement to the visual design. The use of absolute positioning and transforms for centering is an efficient approach.
However, to improve maintainability and consistency, consider using CSS variables for the color and opacity values. Here's a suggested modification:
background-color: var(--tv-Search-input-btn-divider-color, rgba(0, 0, 0, 0.08));This change would allow for easier theming and adjustments in the future.
146-146: Approve the use of CSS variables, but suggest improvements for pixel values.The use of CSS variables for icon color is excellent for maintainability and theming. However, the introduction of hard-coded pixel values for margins and icon size might reduce flexibility.
To further improve maintainability and consistency, consider using CSS variables for all dimension values. Here's a suggested modification:
margin: 0 var(--tv-Search-prefix-icon-margin-right, -8px) 0 var(--tv-Search-prefix-icon-margin-left, 12px); font-size: var(--tv-Search-input-btn-icon-size); fill: var(--tv-Search-input-btn-icon-color);This approach would allow for easier adjustments and maintain consistency with the component's theming system.
Also applies to: 148-150
169-170: Approve the use of CSS variables, but suggest improvement for padding.The use of CSS variables for height, line-height, border color, icon size, and icon color is excellent for maintainability and theming. However, the use of a hard-coded pixel value for padding might reduce flexibility.
To further improve maintainability and consistency, consider using a CSS variable for the padding value as well. Here's a suggested modification:
padding: 0 var(--tv-Search-icon-outer-padding-right, 8px) 0 var(--tv-Search-icon-outer-padding-left, 4px);This approach would allow for easier adjustments and maintain consistency with the component's theming system across all properties.
Also applies to: 174-174, 178-179
235-235: Approve the use of CSS variables for mini and collapse states, with suggestions for improvement.The use of CSS variables for border-radius, width, icon color, and other properties in the mini and collapse state styling is excellent. These changes enhance maintainability and theming capabilities.
However, there are still some hard-coded pixel values that could be replaced with CSS variables for better flexibility. Consider the following modifications:
- Line 242:
width: calc(var(--tv-Search-mini-width) - var(--tv-Search-mini-border-width, 2px));- Line 262:
width: var(--tv-Search-input-height);This approach would allow for easier adjustments and maintain complete consistency with the component's theming system across all properties.
Also applies to: 242-242, 245-245, 253-253, 262-262, 267-267
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- examples/sites/demos/pc/app/search/mini-mode.spec.ts (2 hunks)
- examples/sites/demos/pc/app/search/slot-prefix.spec.ts (1 hunks)
- examples/sites/demos/pc/app/search/transparent-mode.spec.ts (2 hunks)
- packages/theme/src/search/index.less (6 hunks)
- packages/theme/src/search/vars.less (1 hunks)
🔇 Additional comments not posted (14)
examples/sites/demos/pc/app/search/slot-prefix.spec.ts (1)
11-11: Clarify the reason for changing the expected font size.The expected font size for the prefix element has been changed from '14px' to '16px'. While this change might be correct, it's unexpected in a PR described as a refactor, which typically doesn't involve visual changes.
Could you please clarify:
- Is this change intentional?
- If so, what prompted this adjustment?
- Has the actual component been updated to match this new expectation?
To verify if this change is consistent with the component implementation, we can run the following script:
This will help us confirm if the font size change is reflected in the actual component implementation.
examples/sites/demos/pc/app/search/mini-mode.spec.ts (2)
Line range hint
1-29: Overall, the changes maintain test integrity while reflecting UI updates.The modifications to this test file accurately reflect what appears to be a design update to the mini-mode search component. The test structure and coverage remain intact, ensuring that the component's behavior is still thoroughly verified in its various states (collapsed, expanded, and re-collapsed).
These changes align well with the PR objective of refactoring the search theme. They update the expected dimensions without altering the fundamental functionality or test flow, which is consistent with the PR being categorized as a refactor without functional changes.
16-17: LGTM. Verify design specifications.The changes to the expected width and border-radius (from 28px to 32px) for the collapsed state of the search component look good. This update likely reflects a design change in the mini-mode search UI.
To ensure these changes align with the latest design specifications, please run the following verification:
✅ Verification successful
Verified: CSS updates align with test expectations.
The presence of
'32px'inexamples/sites/public/static/css/design-common.cssconfirms that the updated width and border-radius values in the test are consistent with the current design specifications. No issues found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the updated dimensions in design specifications or style files # Search for any design spec files (e.g., Figma exports, CSS, SCSS) that might contain these dimensions rg --type css --type scss --type html '32px' $(fd -e css -e scss -e html) # Search for any comments or documentation about the mini-mode search dimensions rg 'mini.?mode.*search.*dimension' $(fd -e md -e txt)Length of output: 49216
Script:
#!/bin/bash # Description: Verify the updated dimensions in design specifications or style files # Search for '32px' in CSS, SCSS, and HTML files fd -e css -e scss -e html | xargs grep -H '32px' # Search for any comments or documentation about the mini-mode search dimensions fd -e md -e txt | xargs grep -H 'mini.?mode.*search.*dimension'Length of output: 525
examples/sites/demos/pc/app/search/transparent-mode.spec.ts (1)
Line range hint
1-33: Overall, the changes look good and align with the PR objectives.The updates to the CSS property expectations for the
lineelement in both the initial state and after collapsing the search interface are consistent and correct. These changes reflect a design update in the mini mode of the search component.To further improve the test:
- Consider adding comments to explain the design change.
- Think about refactoring repeated expectations into a reusable function for better maintainability.
No additional tests seem necessary based on these changes, as the existing test coverage appears sufficient for the refactored search theme.
packages/theme/src/search/vars.less (3)
13-69: Request information about removed variables.The AI summary mentioned that some variables marked as "hide" were removed, but these removals are not visible in the provided code diff. To ensure a comprehensive review:
Please run the following script to identify any removed variables:
#!/bin/bash # Description: Identify removed variables # Get the current branch name current_branch=$(git rev-parse --abbrev-ref HEAD) # Fetch the base branch (assuming it's main or master) git fetch origin main:main # Show removed lines echo "Removed variables:" git diff main...$current_branch -- packages/theme/src/search/vars.less | grep '^-' | grep -v '^---' | grep -v '^\-\-tv-' # Switch back to the current branch git checkout $current_branchThis will help us understand which variables were removed and assess their impact on the theming system.
15-69: Approve variable naming convention change.The transition from
--ti-to--tv-prefix and the capitalization of "Search" in variable names improve clarity and consistency. This change appears to be part of a broader update to the theming system.To ensure consistency across the project, please run the following script:
#!/bin/bash # Description: Check for any remaining --ti- prefixed variables in LESS files # Search for any remaining --ti- prefixed variables rg --type less '--ti-' packages/theme/src # Count of --tv- prefixed variables in this file for comparison echo "Number of --tv- variables in current file:" rg --type less '--tv-' packages/theme/src/search/vars.less | wc -lThis will help verify that the prefix change has been applied consistently across all relevant files.
15-35: Approve updated variable values with a suggestion for verification.The use of referenced
--tv-variables for values such as font sizes, colors, and spacing promotes consistency and maintainability in the theming system.To ensure all referenced variables are defined and accessible, please run the following script:
#!/bin/bash # Description: Verify referenced variables # Extract all referenced --tv- variables referenced_vars=$(rg --type less 'var\(--tv-[^)]+\)' packages/theme/src/search/vars.less | sed -E 's/.*var\((--tv-[^)]+)\).*/\1/' | sort -u) # Check if each referenced variable is defined echo "Checking referenced variables:" for var in $referenced_vars; do if ! rg --type less "^\s*$var:" packages/theme/src > /dev/null; then echo "- $var might be undefined or defined in another file" fi doneThis script will help identify any potentially undefined variables, ensuring that all references are valid within the theming system.
packages/theme/src/search/index.less (7)
101-101: Clarify the purpose of the specific margin adjustment.A hard-coded margin-right value of -3px has been introduced. While this may serve a specific layout purpose, it's not immediately clear why this exact value is necessary.
Could you please provide more context on why this specific margin adjustment is needed? Consider using a CSS variable for this value to improve maintainability and consistency. For example:
margin-right: var(--tv-Search-input-btn-margin-right, -3px);This approach would allow for easier adjustments in the future if needed.
134-135: Approve the use of CSS variables for icon properties and hover state.The use of CSS variables for icon color and size, as well as the addition of a hover state, are excellent improvements. These changes enhance maintainability, theming capabilities, and user experience by providing visual feedback.
These modifications align well with the overall refactoring objectives and best practices for CSS.
Also applies to: 139-139
157-160: Approve the consistent use of CSS variables for present styling.The use of CSS variables for width, color, line-height, and padding in the present styling is excellent. These changes enhance maintainability and theming capabilities, and are consistent with the overall refactoring effort.
This approach allows for easy adjustments and maintains consistency with the component's theming system. Well done!
206-208: Approve the consistent use of CSS variables for poplist item styling.The use of CSS variables for height, padding, line-height, and font-size in the poplist item styling is excellent. These changes enhance maintainability and theming capabilities, and are consistent with the overall refactoring effort.
This approach allows for easy adjustments and maintains consistency with the component's theming system. Well done!
Also applies to: 210-210
217-217: Approve the use of CSS variables for poplist item hover and icon styling.The use of CSS variables for background color, font-size, and hover icon color in the poplist item hover and icon styling is excellent. These changes enhance maintainability and theming capabilities, and are consistent with the overall refactoring effort.
This approach allows for easy adjustments and maintains consistency with the component's theming system. Good job on maintaining consistency throughout the file.
Also applies to: 222-222, 226-226, 229-229
Line range hint
1-286: Overall, excellent refactoring with minor suggestions for improvement.The changes in this file represent a significant improvement in code maintainability and theming capabilities. The consistent use of CSS variables throughout the file allows for easier customization and ensures a more cohesive design system. This aligns well with the stated objectives of the PR to refactor the search theme.
Key improvements:
- Consistent naming convention for CSS variables (--tv- prefix)
- More descriptive and component-specific variable names
- Extensive use of CSS variables for colors, dimensions, and other properties
Minor suggestions for further improvement:
- Replace remaining hard-coded pixel values with CSS variables
- Fix the typo in the color property for the selector text
- Consider using CSS variables for all padding and margin values
These minor adjustments will further enhance the flexibility and consistency of the component styling.
Great job on this refactoring effort! The changes significantly improve the codebase's maintainability and align well with modern CSS best practices.
27-27: Approve the updated CSS variable naming convention.The changes from
--ti-prefix to--tv-prefix and the more descriptive variable names (e.g.,--tv-Search-input-text-color-disabled) are consistent improvements. These changes align with the refactoring objectives and should enhance code readability and maintainability.To ensure this naming convention is applied consistently across the project, please run the following script:
#!/bin/bash # Description: Check for consistent use of the new CSS variable naming convention # Test: Compare occurrences of old and new variable naming patterns echo "Old variable naming pattern (--ti-):" rg -t less "--ti-\w+" echo "New variable naming pattern (--tv-):" rg -t less "--tv-\w+"This will help verify if the new naming convention has been applied consistently across all relevant files.
Also applies to: 32-33, 38-40, 55-55, 64-67, 72-72, 77-77, 85-85, 88-89, 93-93, 96-96
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