refactor(split): [split] refactor split theme #2194
Conversation
WalkthroughThe changes primarily involve the removal of border styles from various CSS classes across multiple Vue component files. This alteration affects the visual styling of components, specifically those using the 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 (5)
packages/theme/src/split/vars.less (1)
14-23: Consider translating comments to English for better international collaboration.The overall consistency and naming conventions are excellent. The use of the
--tv-Split-prefix and descriptive variable names enhances readability and maintainability.However, the comments are currently in Chinese. For better international collaboration, consider translating these comments to English.
Here's a suggested translation for the comments:
// Panel border color --tv-Split-wrapper-border-color: var(--tv-color-border); // Panel split line trigger icon line color --tv-Split-trigger-icon-line-color: var(--tv-color-icon); // Panel split line trigger icon line hover color --tv-Split-trigger-icon-line-color-hover: var(--tv-color-icon-hover); // Panel border radius size --tv-Split-wrapper-radius-size: var(--tv-border-radius-lg); // Simple mode split line hover background color --tv-Split-trigger-line-bg-hover: var(--tv-color-border-active-control);examples/sites/demos/pc/app/split/split-threshold.spec.ts (1)
Line range hint
1-24: Consider adding more comprehensive test cases.While the changes to the assertions make the tests more precise, they might also make them more brittle. Additionally, the current test doesn't fully verify the threshold behavior of the split panel.
Consider adding the following test cases:
- Test that the panel doesn't resize below the minimum threshold.
- Test that the panel doesn't resize above the maximum threshold.
- Test the behavior when trying to resize exactly at the threshold limits.
For example:
// Test minimum threshold await triggerBtn.hover() await page.mouse.down() await page.mouse.move(x, y + 1000) // Move way past the threshold await page.mouse.up() const { height: minHeight } = await topPanel.boundingBox() await expect(minHeight).toBeGreaterThanOrEqual(50) // Assuming 50 is the min threshold // Similar tests for maximum threshold and at-threshold behaviorThese additional tests will ensure that the split functionality works correctly within its defined limits.
examples/sites/demos/pc/app/split/nested-use.spec.ts (1)
Line range hint
1-36: Summary of changes and alignment with PR objectivesThe changes in this file consistently modify the assertion logic for component dimensions after user interactions. While these changes align with the general goal of refactoring mentioned in the PR objectives, there are a few points to consider:
- The modifications potentially make the tests more strict, which could catch previously undetected issues but might also introduce false positives.
- The refactoring approach could be further improved to enhance maintainability and flexibility of the tests.
To better align with the refactoring objectives:
- Consider implementing a more generalized solution for these assertions, as suggested in the previous comments.
- Ensure that these changes are consistent across all relevant test files in the project.
- Update the documentation to reflect any new patterns or helpers introduced during this refactoring.
- If these changes are part of a larger refactoring effort for the split theme, consider providing more context in the PR description about the specific goals and scope of the refactoring.
These steps will help ensure that the refactoring not only improves the current file but also contributes to the overall maintainability and consistency of the project's test suite.
packages/theme/src/split/index.less (2)
30-30: Translate comment to EnglishThe comment is currently in Chinese:
// split嵌套使用内部不需要圆角和边框For better understanding among the team and consistency, please translate the comment to English.
76-76: Specify transitioned properties for better performanceUsing
transition: all 0.1s;affects all properties, which can lead to unintended animations and performance issues. It's better to specify only the properties that need to be transitioned.Apply this diff to specify the properties:
-transition: all 0.1s; +transition: transform 0.1s;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (41)
- examples/sites/demos/pc/app/split/basic-usage-composition-api.vue (0 hunks)
- examples/sites/demos/pc/app/split/basic-usage.spec.ts (1 hunks)
- examples/sites/demos/pc/app/split/basic-usage.vue (0 hunks)
- examples/sites/demos/pc/app/split/collapsible-composition-api.vue (0 hunks)
- examples/sites/demos/pc/app/split/collapsible.vue (0 hunks)
- examples/sites/demos/pc/app/split/disabled-drag-composition-api.vue (0 hunks)
- examples/sites/demos/pc/app/split/disabled-drag.vue (0 hunks)
- examples/sites/demos/pc/app/split/event-click-composition-api.vue (0 hunks)
- examples/sites/demos/pc/app/split/event-click.vue (0 hunks)
- examples/sites/demos/pc/app/split/horizontal-collapse-composition-api.vue (0 hunks)
- examples/sites/demos/pc/app/split/horizontal-collapse-left-top-composition-api.vue (0 hunks)
- examples/sites/demos/pc/app/split/horizontal-collapse-left-top.vue (0 hunks)
- examples/sites/demos/pc/app/split/horizontal-collapse.vue (0 hunks)
- examples/sites/demos/pc/app/split/left-right-slot-composition-api.vue (0 hunks)
- examples/sites/demos/pc/app/split/left-right-slot.vue (0 hunks)
- examples/sites/demos/pc/app/split/moveend-event-composition-api.vue (0 hunks)
- examples/sites/demos/pc/app/split/moveend-event.vue (0 hunks)
- examples/sites/demos/pc/app/split/movestart-event-composition-api.vue (0 hunks)
- examples/sites/demos/pc/app/split/movestart-event.vue (0 hunks)
- examples/sites/demos/pc/app/split/moving-event-composition-api.vue (0 hunks)
- examples/sites/demos/pc/app/split/moving-event.vue (0 hunks)
- examples/sites/demos/pc/app/split/nested-use-composition-api.vue (0 hunks)
- examples/sites/demos/pc/app/split/nested-use.spec.ts (2 hunks)
- examples/sites/demos/pc/app/split/nested-use.vue (0 hunks)
- examples/sites/demos/pc/app/split/split-mode-composition-api.vue (0 hunks)
- examples/sites/demos/pc/app/split/split-mode.spec.ts (1 hunks)
- examples/sites/demos/pc/app/split/split-mode.vue (0 hunks)
- examples/sites/demos/pc/app/split/split-threshold-composition-api.vue (0 hunks)
- examples/sites/demos/pc/app/split/split-threshold.spec.ts (1 hunks)
- examples/sites/demos/pc/app/split/split-threshold.vue (0 hunks)
- examples/sites/demos/pc/app/split/three-areas-composition-api.vue (0 hunks)
- examples/sites/demos/pc/app/split/three-areas.vue (0 hunks)
- examples/sites/demos/pc/app/split/top-bottom-slot-composition-api.vue (0 hunks)
- examples/sites/demos/pc/app/split/top-bottom-slot.vue (0 hunks)
- examples/sites/demos/pc/app/split/trigger-simple-composition-api.vue (0 hunks)
- examples/sites/demos/pc/app/split/trigger-simple.spec.ts (1 hunks)
- examples/sites/demos/pc/app/split/trigger-simple.vue (0 hunks)
- examples/sites/demos/pc/app/split/trigger-slot-composition-api.vue (0 hunks)
- examples/sites/demos/pc/app/split/trigger-slot.vue (0 hunks)
- packages/theme/src/split/index.less (7 hunks)
- packages/theme/src/split/vars.less (1 hunks)
💤 Files not reviewed due to no reviewable changes (34)
- examples/sites/demos/pc/app/split/basic-usage-composition-api.vue
- examples/sites/demos/pc/app/split/basic-usage.vue
- examples/sites/demos/pc/app/split/collapsible-composition-api.vue
- examples/sites/demos/pc/app/split/collapsible.vue
- examples/sites/demos/pc/app/split/disabled-drag-composition-api.vue
- examples/sites/demos/pc/app/split/disabled-drag.vue
- examples/sites/demos/pc/app/split/event-click-composition-api.vue
- examples/sites/demos/pc/app/split/event-click.vue
- examples/sites/demos/pc/app/split/horizontal-collapse-composition-api.vue
- examples/sites/demos/pc/app/split/horizontal-collapse-left-top-composition-api.vue
- examples/sites/demos/pc/app/split/horizontal-collapse-left-top.vue
- examples/sites/demos/pc/app/split/horizontal-collapse.vue
- examples/sites/demos/pc/app/split/left-right-slot-composition-api.vue
- examples/sites/demos/pc/app/split/left-right-slot.vue
- examples/sites/demos/pc/app/split/moveend-event-composition-api.vue
- examples/sites/demos/pc/app/split/moveend-event.vue
- examples/sites/demos/pc/app/split/movestart-event-composition-api.vue
- examples/sites/demos/pc/app/split/movestart-event.vue
- examples/sites/demos/pc/app/split/moving-event-composition-api.vue
- examples/sites/demos/pc/app/split/moving-event.vue
- examples/sites/demos/pc/app/split/nested-use-composition-api.vue
- examples/sites/demos/pc/app/split/nested-use.vue
- examples/sites/demos/pc/app/split/split-mode-composition-api.vue
- examples/sites/demos/pc/app/split/split-mode.vue
- examples/sites/demos/pc/app/split/split-threshold-composition-api.vue
- examples/sites/demos/pc/app/split/split-threshold.vue
- examples/sites/demos/pc/app/split/three-areas-composition-api.vue
- examples/sites/demos/pc/app/split/three-areas.vue
- examples/sites/demos/pc/app/split/top-bottom-slot-composition-api.vue
- examples/sites/demos/pc/app/split/top-bottom-slot.vue
- examples/sites/demos/pc/app/split/trigger-simple-composition-api.vue
- examples/sites/demos/pc/app/split/trigger-simple.vue
- examples/sites/demos/pc/app/split/trigger-slot-composition-api.vue
- examples/sites/demos/pc/app/split/trigger-slot.vue
🔇 Additional comments (18)
examples/sites/demos/pc/app/split/split-mode.spec.ts (1)
11-11: Clarification needed: Width comparison adjustmentThe assertion for comparing the split element's width with the center button's width has been modified. Previously, it expected the widths to be approximately equal, but now it expects the split element to be 2 units wider than the center button.
Could you please clarify the reason for this adjustment? It's important to verify if this change is intentional and aligns with the component's design specifications.
To ensure this change doesn't introduce unexpected visual regressions, please consider the following:
- Review the component's design specifications to confirm if this 2-unit difference is expected.
- Visually inspect the rendered component to ensure it appears as intended.
- If possible, add a comment explaining the reason for this 2-unit difference to improve code maintainability.
examples/sites/demos/pc/app/split/trigger-simple.spec.ts (1)
19-19: 🛠️ Refactor suggestionConsider the implications of using exact equality with ceiling values.
The change from
toBeCloseTototoEqualwithMath.ceil()makes the assertion more strict. While this might increase accuracy by ensuring exact pixel values, it could potentially make the test more brittle.Consider the following points:
- The new assertion doesn't allow for small floating-point discrepancies, which might occur due to sub-pixel calculations during the drag operation.
- Different browsers or systems might have slight variations in rendering, which this strict equality doesn't account for.
To balance accuracy and test stability, consider using
toBeCloseTowith a smaller epsilon value:await expect(afterMove).toBeCloseTo(afterWidth, 0); // Allows for a difference of less than 1This approach would still catch significant discrepancies while allowing for minor, inconsequential differences.
To ensure this change doesn't introduce flakiness, run the following script to check for any existing uses of
toBeCloseToin similar contexts:If other split-related tests use
toBeCloseTo, it might be worth maintaining consistency across these tests.packages/theme/src/split/vars.less (3)
13-13: LGTM: Function name and structure are appropriate.The function name
.inject-Split-vars()clearly indicates its purpose, and the use of a Less mixin is a good practice for defining reusable sets of CSS variables.
15-23: LGTM: CSS variable definitions are well-structured and meaningful.The five CSS variables are appropriately named and assigned values:
--tv-Split-wrapper-border-color--tv-Split-trigger-icon-line-color--tv-Split-trigger-icon-line-color-hover--tv-Split-wrapper-radius-size--tv-Split-trigger-line-bg-hoverEach variable uses the
--tv-Split-prefix for consistency and is assigned a value using another CSS variable, which is a good practice for maintaining a flexible theming system.
13-23: Verify the impact of these changes on the rest of the codebase.The introduction of
.inject-Split-vars()and its new set of variables represents a significant change in the theming approach for the split panel component. While the new variables provide a more focused and potentially more flexible way to style the component, it's important to ensure that these changes don't negatively impact other parts of the codebase.Please run the following script to check for any potential issues:
This script will help identify:
- Any remaining usage of the old split variables
- Places where the new
.inject-Split-vars()function is being used- Files related to the split component that might need updating
Please review the results and update any affected files accordingly.
✅ Verification successful
Old Split Variables are No Longer in Use
The verification confirms that there is no usage of old split variables in the codebase, and the new
.inject-Split-vars()function is properly implemented in the relevant files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of old split variables and the new inject function # Test 1: Check for usage of old split variables echo "Checking for usage of old split variables:" rg --type less '\-\-tv-(split-font-size|split-bg-color|split-line-bg-color|split-btn-wrapper-width|split-btn-wrapper-height)' # Test 2: Check for usage of the new inject function echo "Checking for usage of the new inject function:" rg --type less '\.inject-Split-vars\(\)' # Test 3: Check for files that might need updating echo "Files that might need updating:" rg --type less 'split'Length of output: 9209
packages/theme/src/split/index.less (13)
21-21: Update mixin to.inject-Split-vars()Replacing
.component-css-vars-split()with.inject-Split-vars()ensures that the Split component uses the latest theming variables, enhancing maintainability and consistency across the theme.
27-28: Utilize CSS variables for wrapper stylingApplying CSS variables for
border-radiusandborderproperties (--tv-Split-wrapper-radius-sizeand--tv-Split-wrapper-border-color) promotes consistency and allows for easier theming.
85-86: Adjust trigger bar dimensions and positioningUpdating the
widthto16pxand adjusting thetransformimproves the alignment and click area of the vertical trigger bar.
90-90: Use CSS variable for icon fill colorReplacing hardcoded fill color with
var(--tv-Split-trigger-icon-line-color)enhances theming flexibility and consistency.
100-101: Adjust SVG transform and use theme variable for fillModifying the SVG
transformand utilizingvar(--tv-Split-trigger-icon-line-color)for the fill color in the horizontal trigger ensures consistency with the vertical trigger styling.
107-109: Define vertical trigger dimensions and background using variablesSetting
width,height, andbackgroundwith CSS variables ensures the vertical trigger styling is consistent and themeable.
115-115: Use theme variable for trigger bar backgroundApplying
var(--tv-Split-wrapper-border-color)for the trigger bar background maintains consistent theming.
122-122: Set dimensions for horizontal triggerDefining
heightandwidthfor the horizontal trigger ensures it aligns with the design specifications.
130-130: Apply theme variable to horizontal trigger bar backgroundUsing
var(--tv-Split-wrapper-border-color)for the background improves consistency and theming.
175-175: Update cursor style tonot-allowedfor disabled triggersChanging the cursor to
not-allowedon disabled trigger controls enhances user experience by clearly indicating non-interactive elements.Also applies to: 243-243
278-278: Set SVG fill color using theme variableUsing
var(--tv-Split-trigger-icon-line-color)for the SVG fill color ensures consistency across themes.
285-285: Set SVG fill color on hover using theme variableApplying
var(--tv-Split-trigger-icon-line-color-hover)for the hover state fill color provides visual feedback consistent with the theme.
319-319: Set trigger line background on hover using theme variableUsing
var(--tv-Split-trigger-line-bg-hover)for the background on hover enhances theming and user interaction feedback.
| @@ -17,5 +17,5 @@ test('基本用法', async ({ page }) => { | |||
| const afterMove = leftPanelWidth + 100 - triggerBtnWidth / 2 | |||
| const { width: afterWidth } = await leftPanel.boundingBox() | |||
| // 小数点后一位相同即可 | |||
| await expect(afterMove).toBeCloseTo(afterWidth, 1) | |||
| await expect(Math.ceil(afterMove)).toEqual(Math.ceil(afterWidth)) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reconsider the assertion change for width comparison
The change from toBeCloseTo to toEqual with Math.ceil might make the test more brittle and less representative of the actual component behavior. Consider the following alternatives:
-
Retain
toBeCloseTowith a small tolerance:await expect(afterMove).toBeCloseTo(afterWidth, 1);
-
If exact equality is required, add a comment explaining why:
// Exact pixel equality is required for this component await expect(Math.round(afterMove)).toEqual(Math.round(afterWidth));
-
Use a custom matcher with a small absolute tolerance:
await expect(Math.abs(afterMove - afterWidth)).toBeLessThan(0.5);
These alternatives provide more flexibility and clarity in handling potential floating-point discrepancies.
| @@ -13,13 +13,13 @@ test('面板阈值', async ({ page }) => { | |||
| await page.mouse.move(x, y - 100) | |||
| await page.mouse.up() | |||
| const { height: afterHeight } = await topPanel.boundingBox() | |||
| await expect(50).toBeCloseTo(afterHeight, 1) | |||
| await expect(50).toEqual(Math.ceil(afterHeight)) | |||
There was a problem hiding this comment.
Consider the implications of using exact equality for panel height.
The change from toBeCloseTo to toEqual with Math.ceil makes the test more strict. While this ensures precise behavior, it might lead to brittle tests if there are small variations in panel sizes due to browser rendering differences.
Consider using a small tolerance or range check instead. For example:
const expectedHeight = 50;
await expect(afterHeight).toBeGreaterThanOrEqual(expectedHeight - 1);
await expect(afterHeight).toBeLessThanOrEqual(expectedHeight + 1);This approach maintains strictness while allowing for minor rendering variations.
| await page.waitForTimeout(300) | ||
| // 向下滑动时判断下面面板高度是否为80 | ||
| await triggerBtn.hover() | ||
| await page.mouse.down() | ||
| await page.mouse.move(x, y + 200) | ||
| const { height: bottomMove } = await bottomPanel.boundingBox() | ||
| await page.mouse.up() | ||
| await expect(80).toBeCloseTo(bottomMove, 1) | ||
| await expect(80).toEqual(Math.ceil(bottomMove)) |
There was a problem hiding this comment.
Consistent change in assertion, but consider variable naming.
The change to use toEqual with Math.ceil is consistent with the previous assertion. However, there's a naming issue to address.
- The variable
bottomMovesuggests movement, but it's storing the height. Consider renaming it tobottomHeightfor clarity:
const { height: bottomHeight } = await bottomPanel.boundingBox()
await expect(80).toEqual(Math.ceil(bottomHeight))- As mentioned in the previous comment, consider using a tolerance range instead of exact equality to make the test more robust against minor rendering variations.
| @@ -18,7 +18,7 @@ test('嵌套使用', async ({ page }) => { | |||
| // 移动之后的宽度为:移动之前的宽度+50-分割线宽度的一半 | |||
| const afterMove = leftWidth + 50 - centerWidth / 2 | |||
| const { width: afterWidth } = await leftDiv.boundingBox() | |||
| await expect(afterMove).toBeCloseTo(afterWidth, 1) | |||
| await expect(Math.ceil(afterMove)).toEqual(Math.ceil(afterWidth)) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider the implications of using Math.ceil and toEqual.
The change from toBeCloseTo to Math.ceil and toEqual alters the assertion's behavior:
- It makes the test more strict by requiring exact equality after rounding up.
- It might catch issues previously masked by floating-point imprecisions.
- However, it could make the test more brittle if small variations are expected and acceptable.
Consider the following alternatives:
- Use
Math.roundinstead ofMath.ceilto handle cases where the value is closer to the lower integer. - Implement a custom matcher that allows for a small tolerance while still using integer comparisons.
Example of a custom matcher:
expect(Math.round(afterMove)).toEqual(expect.closeTo(Math.round(afterWidth), 1));This approach would maintain integer comparisons while allowing for a small tolerance.
| @@ -32,6 +32,5 @@ | |||
| await page.mouse.up() | |||
| const { height: afterHeight } = await topDiv.boundingBox() | |||
| const topMove = topHeight - 30 - verticalHeight / 2 | |||
| await expect(topMove).toBeCloseTo(afterHeight, 1) | |||
| await page.waitForTimeout(300) | |||
| await expect(Math.ceil(topMove)).toEqual(Math.ceil(afterHeight)) | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consistent change, consider generalizing the approach.
The change here is consistent with the previous modification, which is good for maintaining uniformity in the test assertions.
To improve maintainability and reduce duplication, consider:
- Creating a custom matcher or helper function for these comparisons. This would centralize the logic and make it easier to adjust in the future.
Example:
function expectRoundedEqual(actual: number, expected: number) {
expect(Math.round(actual)).toEqual(Math.round(expected));
}
// Usage
expectRoundedEqual(topMove, afterHeight);- If this pattern is used across multiple test files, consider adding it to a shared test utilities file.
This approach would make the tests more readable and easier to maintain, especially if you need to adjust the comparison logic in the future.
| & & { | ||
| border-radius: 0; | ||
| border: none; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review nested selector & & for specificity
Using the nested selector & & may increase CSS specificity, which can make overriding styles more difficult. Consider whether this level of specificity is necessary or if there's an alternative approach.
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