feat(dropdown): [dropdown,action-menu,select] Additional features xdesign theme adaptation #2140
Conversation
|
[e2e-test-warn] The title of the Pull request should look like "fix(vue-renderless): [action-menu, alert] fix xxx bug". Please make sure you've read our contributing guide |
MomoPoppy
changed the title
WalkthroughThe changes introduced in this pull request enhance the functionality of various UI components, particularly the Changes
Possibly related PRs
Suggested reviewers
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (5)
Files skipped from review as they are similar to previous changes (3)
Additional comments not posted (3)
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: 4
Outside diff range and nitpick comments (2)
examples/sites/demos/pc/app/slider/show-input.spec.ts (1)
20-20: Consider the impact on test execution time.The added delay after hovering over the slider block is a good practice to improve test stability by allowing time for the UI to update. However, please keep in mind that extensively adding similar delays across the test suite might noticeably increase the overall execution time.
Consider extracting the delay value into a constant or configuration variable to maintain consistency and ease of adjustment across the test suite.
const INTERACTION_DELAY = 200; // ... await page.waitForTimeout(INTERACTION_DELAY);packages/renderless/src/slider/index.ts (1)
Line range hint
644-648: The changes to thehandleSlotInputfunction look good.The simplification of the
isLeftInputparameter declaration by removing the explicit type annotation is acceptable, as it does not affect the functionality of the function.Consider adding a type annotation to the
isLeftInputparameter for improved code readability:- (event: Event, isLeftInput = true): void => { + (event: Event, isLeftInput: boolean = true): void => {
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- examples/sites/demos/pc/app/dropdown/basic-usage.spec.ts (2 hunks)
- examples/sites/demos/pc/app/dropdown/disabled.spec.ts (3 hunks)
- examples/sites/demos/pc/app/dropdown/size.spec.ts (1 hunks)
- examples/sites/demos/pc/app/dropdown/split-button.spec.ts (1 hunks)
- examples/sites/demos/pc/app/select/automatic-dropdown.spec.ts (1 hunks)
- examples/sites/demos/pc/app/select/copy-multi.spec.ts (2 hunks)
- examples/sites/demos/pc/app/select/input-box-type.spec.ts (3 hunks)
- examples/sites/demos/pc/app/select/multiple.spec.ts (2 hunks)
- examples/sites/demos/pc/app/select/nest-grid-remote.spec.ts (1 hunks)
- examples/sites/demos/pc/app/select/nest-grid.spec.ts (1 hunks)
- examples/sites/demos/pc/app/select/nest-radio-grid-much-data.spec.ts (1 hunks)
- examples/sites/demos/pc/app/select/size.spec.ts (4 hunks)
- examples/sites/demos/pc/app/slider/show-input.spec.ts (2 hunks)
- packages/renderless/src/slider/index.ts (2 hunks)
Additional comments not posted (25)
examples/sites/demos/pc/app/dropdown/size.spec.ts (1)
15-19: Temporarily disabling the assertions is a good practice.Commenting out the assertions while the button component's size adaptation is being worked on is a good practice to avoid failing tests. The TODO comment serves as a reminder to re-enable the assertions once the button component's size adaptation is complete.
Please ensure that the assertions are re-enabled once the button component's size adaptation is complete to avoid having an incomplete test.
examples/sites/demos/pc/app/slider/show-input.spec.ts (1)
7-7: Verify the consistency of the slider input structure.The change in the locator for the slider input is appropriate given the updated structure of the component. However, please ensure that this change is consistent across the application and does not break other tests or code that relies on the previous locator.
Run the following script to verify the usage of the previous locator:
examples/sites/demos/pc/app/dropdown/split-button.spec.ts (2)
15-15: LGTM!The change in background color for the
textBtnelement is consistent with the PR summary and looks good.
17-17: LGTM!The change in background color for the
dropDownBtnelement is consistent with the PR summary and looks good.examples/sites/demos/pc/app/select/automatic-dropdown.spec.ts (1)
12-12: LGTM! The change aligns with the PR objective.The modification to the expected border color of the input element upon focus is a visual styling change that aligns with the goal of adapting the components to the xdesign theme. The test case continues to verify the expected behavior of the dropdown remaining hidden when the input cannot be searched.
examples/sites/demos/pc/app/select/nest-radio-grid-much-data.spec.ts (3)
26-26: LGTM!The change in the expected class name for the second row is appropriate and aligns with the updated styling or selection behavior.
27-27: LGTM!The reduction in the expected row count from 8 to 6 is appropriate and aligns with the changes in the data being displayed or the pagination behavior.
29-34: LGTM!The adjustments in the scroll actions, targeting the 5th row instead of the 7th row, are appropriate and align with the changes in the data or pagination. The consistent expected row count of 6 after each scroll indicates that the pagination or data loading behavior remains stable.
examples/sites/demos/pc/app/dropdown/basic-usage.spec.ts (2)
16-19: LGTM!The update to the expected SVG path attribute aligns the test with the modified visual representation of the dropdown arrow. This change ensures that the test accurately verifies the appearance of the dropdown icon.
33-34: LGTM!The updates to the expected color and background-color values align the test with the modified styling of the dropdown menu items when hovered over. These changes ensure that the test accurately verifies the visual feedback provided to the user when interacting with the dropdown menu.
examples/sites/demos/pc/app/select/multiple.spec.ts (2)
13-25: LGTM!The changes in the test case align with the expected behavior of a multi-select component. The adjustments to the tag count expectations accurately reflect the selection and deselection of options, including the "All" option. The test case ensures that the component correctly handles the updates to the selected tags based on user interactions.
40-51: LGTM!The changes in the test case simplify the assertions for option classes, making the test more readable and maintainable. The specific assertions for each option's class accurately reflect the expected behavior of a multi-select component with a selection limit. The test case ensures that the component correctly applies the limit and disables the appropriate options based on the number of selections made.
examples/sites/demos/pc/app/select/copy-multi.spec.ts (2)
60-64: Investigate and fix the root cause of the element positioning issue.The test case is hovering over the select element. However, the click action on the copy icon and subsequent paste and value assertion are commented out due to an issue with element positioning during test execution.
While commenting out the affected code serves as a temporary workaround, it prevents the test case from fully verifying the copy functionality with a custom separator. Please investigate and fix the root cause of the element positioning issue to enable the commented-out code and ensure a reliable test suite.
Line range hint
1-30: LGTM!The test case logic for copying a single tag is correct, and the implementation is accurate.
examples/sites/demos/pc/app/select/size.spec.ts (1)
12-12: LGTM!The updated height expectation for the default size input element looks good.
examples/sites/demos/pc/app/dropdown/disabled.spec.ts (4)
14-14: LGTM!The color value update for the disabled dropdown trigger is consistent with the changes described in the PR summary.
15-15: LGTM!The SVG fill color update for the disabled dropdown icon is consistent with the changes described in the PR summary.
38-39: LGTM!The color value updates for the disabled text and trigger buttons are consistent with the changes described in the PR summary.
41-41: LGTM!The color value updates for the disabled trigger button icon and dropdown menu item are consistent with the changes described in the PR summary.
Also applies to: 60-60
examples/sites/demos/pc/app/select/input-box-type.spec.ts (3)
16-17: LGTM!The updates to the expected border color and SVG fill color for the default underline style test case look good. The changes align with the overall visual styling updates mentioned in the PR summary.
37-39: LGTM!The updates to the expected border color and SVG fill color for the disabled underline style test case look good. The changes align with the overall visual styling updates mentioned in the PR summary.
62-63: LGTM!The updates to the expected border color and caret fill color for the multiple selection underline style test case look good. The changes align with the overall visual styling updates mentioned in the PR summary.
examples/sites/demos/pc/app/select/nest-grid.spec.ts (1)
24-24: LGTM!The change in the assertion aligns with the naming convention of the Tiny Vue framework and improves the test case by updating the expectation to match the actual class structure of the selected row.
examples/sites/demos/pc/app/select/nest-grid-remote.spec.ts (1)
31-31: LGTM!The update to the expected class for the selected row in the grid is appropriate and keeps the test case in sync with the component's behavior.
packages/renderless/src/slider/index.ts (1)
Line range hint
654-661: The changes to theinputOnChangefunction are valid.The modification of the parameter type from
Eventtostringand the corresponding update to the internal logic to validatecurrentValueinstead ofevent.target.valuealign with the new expectation of receiving the input value directly.Verify that the calling code has been updated to pass the input value as a string instead of an event object to ensure compatibility with the modified function signature.
Run the following script to search for occurrences of
inputOnChangeand review the arguments passed to it:
| const copyIcon = select.locator('.tiny-select__copy .tiny-svg') | ||
|
|
||
| await page.waitForTimeout(200) | ||
| await select.hover() | ||
| await select.locator('.tiny-select__copy > .tiny-svg').click() | ||
|
|
||
| await copyValueInput.press('Control+V') | ||
| await expect(copyValueInput).toHaveValue('北京,上海') | ||
| await expect(copyIcon).toBeVisible() | ||
| // TODO: 因为执行测试用例时，官网抖动导致定位不通过 | ||
| // await copyIcon.click() | ||
| // await copyValueInput.press('Control+V') | ||
| // await expect(copyValueInput).toHaveValue('北京,上海') |
There was a problem hiding this comment.
Investigate and fix the root cause of the element positioning issue.
The test case is correctly locating the copy icon and checking its visibility. However, the click action on the copy icon and subsequent paste and value assertion are commented out due to an issue with element positioning during test execution.
While commenting out the affected code serves as a temporary workaround, it prevents the test case from fully verifying the copy functionality. Please investigate and fix the root cause of the element positioning issue to enable the commented-out code and ensure a reliable test suite.
| // TODO: 此处继承input 尺寸的话，应该是32 | ||
| await expect(input.locator('.tiny-input__inner')).toHaveCSS('height', '40px') |
There was a problem hiding this comment.
Address the TODO comment and update the height expectation if applicable.
The TODO comment suggests that the expected height for the medium size input should be '32px' if inheriting from the input size. However, the current expectation is set to '40px'.
Please clarify whether the height should inherit from the input size or remain at '40px', and update the test case accordingly.
| // TODO: 此处继承input 尺寸的话，应该是28 | ||
| expect(height).toBeCloseTo(32, 1) |
There was a problem hiding this comment.
Address the TODO comment and update the height expectation if applicable.
The TODO comment suggests that the expected height for the small size input should be '28px' if inheriting from the input size. However, the current expectation is set to '32px' using the toBeCloseTo matcher.
Please clarify whether the height should inherit from the input size or remain at '32px', and update the test case accordingly.
| // TODO: 此处继承input 尺寸的话，应该是24 | ||
| expect(height).toBeCloseTo(32, 1) |
There was a problem hiding this comment.
Address the TODO comment and update the height expectation if applicable.
The TODO comment suggests that the expected height for the mini size input should be '24px' if inheriting from the input size. However, the current expectation is set to '32px' using the toBeCloseTo matcher.
Please clarify whether the height should inherit from the input size or remain at '32px', and update the test case accordingly.
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
tiny-selectcomponent, allowing a maximum of two selections.show-limit-textproperty for selection limits.Bug Fixes
Style
Chores