fix: search no scroll #532
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #532 +/- ##
=======================================
Coverage 99.67% 99.67%
=======================================
Files 23 23
Lines 609 611 +2
Branches 184 185 +1
=======================================
+ Hits 607 609 +2
Misses 2 2 ☔ View full report in Codecov by Sentry. |
afc163
reviewed
Sep 2, 2024
Walkthrough此次更改涉及移除 Changes
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: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- src/OptionList/Column.tsx (3 hunks)
- src/OptionList/List.tsx (3 hunks)
Additional comments not posted (2)
src/OptionList/Column.tsx (1)
Line range hint
118-126: 代码变更看起来不错，但需要验证搜索功能删除
searchValue检查简化了triggerOpenPath函数的逻辑，这与修复搜索滚动问题的 PR 目标一致。请运行以下脚本来验证这个变更对搜索功能的影响：
src/OptionList/List.tsx (1)
Line range hint
169-182: 优化搜索时的滚动行为这个改动解决了搜索时不必要滚动的问题，符合PR的目标。
注意：这个改动与之前的评论建议相似。可以考虑采用更简洁的条件检查：
- if (searchValue) { - return; - } + if (!searchValue) {这样可以减少代码的缩进层级，提高可读性。
Comment on lines
+1025
to
+1103
| it('hover + search', () => { | ||
| let getOffesetTopTimes = 0; | ||
| const spyElement = spyElementPrototypes(HTMLElement, { | ||
| offsetTop: { | ||
| get: () => (getOffesetTopTimes++ % 2 === 0 ? 100 : 0), | ||
| }, | ||
| scrollTop: { | ||
| get: () => 0, | ||
| }, | ||
| offsetHeight: { | ||
| get: () => 10, | ||
| }, | ||
| }); | ||
|
|
||
| const wrapper = render( | ||
| <Cascader | ||
| expandTrigger="hover" | ||
| options={[ | ||
| { | ||
| label: 'Women Clothing', | ||
| value: '1', | ||
| children: [ | ||
| { | ||
| label: 'Women Tops, Blouses & Tee', | ||
| value: '11', | ||
| children: [ | ||
| { | ||
| label: 'Women T-Shirts', | ||
| value: '111', | ||
| }, | ||
| { | ||
| label: 'Women Tops', | ||
| value: '112', | ||
| }, | ||
| { | ||
| label: 'Women Tank Tops & Camis', | ||
| value: '113', | ||
| }, | ||
| { | ||
| label: 'Women Blouses', | ||
| value: '114', | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| label: 'Women Suits', | ||
| value: '2', | ||
| children: [ | ||
| { | ||
| label: 'Women Suit Pants', | ||
| value: '21', | ||
| }, | ||
| { | ||
| label: 'Women Suit Sets', | ||
| value: '22', | ||
| }, | ||
| { | ||
| label: 'Women Blazers', | ||
| value: '23', | ||
| }, | ||
| ], | ||
| }, | ||
| ], | ||
| }, | ||
| ]} | ||
| showSearch | ||
| checkable | ||
| open | ||
| />, | ||
| ); | ||
| fireEvent.change(wrapper.container.querySelector('input') as HTMLElement, { | ||
| target: { value: 'w' }, | ||
| }); | ||
| const items = wrapper.container.querySelectorAll('.rc-cascader-menu-item'); | ||
| fireEvent.mouseEnter(items[9]); | ||
| expect(mockScrollTo).toHaveBeenCalledTimes(0); | ||
|
|
||
| spyElement.mockRestore(); | ||
| }); |
There was a problem hiding this comment.
新测试用例看起来不错，但可以考虑添加更多断言。
新添加的"hover + search"测试用例很好地覆盖了悬停和搜索功能的交互。代码结构清晰，模拟了用户交互，并验证了scrollTo函数未被调用。
建议添加更多断言来增强测试的可靠性。例如，可以验证搜索结果的数量，或者检查特定元素是否存在于DOM中。这将使测试更加健壮。
expect(mockScrollTo).toHaveBeenCalledTimes(0);
+expect(items).toHaveLength(10); // 验证搜索结果的数量
+expect(wrapper.container.querySelector('.rc-cascader-menu-item-active')).toBeTruthy(); // 检查是否有活动项Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('hover + search', () => { | |
| let getOffesetTopTimes = 0; | |
| const spyElement = spyElementPrototypes(HTMLElement, { | |
| offsetTop: { | |
| get: () => (getOffesetTopTimes++ % 2 === 0 ? 100 : 0), | |
| }, | |
| scrollTop: { | |
| get: () => 0, | |
| }, | |
| offsetHeight: { | |
| get: () => 10, | |
| }, | |
| }); | |
| const wrapper = render( | |
| <Cascader | |
| expandTrigger="hover" | |
| options={[ | |
| { | |
| label: 'Women Clothing', | |
| value: '1', | |
| children: [ | |
| { | |
| label: 'Women Tops, Blouses & Tee', | |
| value: '11', | |
| children: [ | |
| { | |
| label: 'Women T-Shirts', | |
| value: '111', | |
| }, | |
| { | |
| label: 'Women Tops', | |
| value: '112', | |
| }, | |
| { | |
| label: 'Women Tank Tops & Camis', | |
| value: '113', | |
| }, | |
| { | |
| label: 'Women Blouses', | |
| value: '114', | |
| }, | |
| ], | |
| }, | |
| { | |
| label: 'Women Suits', | |
| value: '2', | |
| children: [ | |
| { | |
| label: 'Women Suit Pants', | |
| value: '21', | |
| }, | |
| { | |
| label: 'Women Suit Sets', | |
| value: '22', | |
| }, | |
| { | |
| label: 'Women Blazers', | |
| value: '23', | |
| }, | |
| ], | |
| }, | |
| ], | |
| }, | |
| ]} | |
| showSearch | |
| checkable | |
| open | |
| />, | |
| ); | |
| fireEvent.change(wrapper.container.querySelector('input') as HTMLElement, { | |
| target: { value: 'w' }, | |
| }); | |
| const items = wrapper.container.querySelectorAll('.rc-cascader-menu-item'); | |
| fireEvent.mouseEnter(items[9]); | |
| expect(mockScrollTo).toHaveBeenCalledTimes(0); | |
| spyElement.mockRestore(); | |
| }); | |
| it('hover + search', () => { | |
| let getOffesetTopTimes = 0; | |
| const spyElement = spyElementPrototypes(HTMLElement, { | |
| offsetTop: { | |
| get: () => (getOffesetTopTimes++ % 2 === 0 ? 100 : 0), | |
| }, | |
| scrollTop: { | |
| get: () => 0, | |
| }, | |
| offsetHeight: { | |
| get: () => 10, | |
| }, | |
| }); | |
| const wrapper = render( | |
| <Cascader | |
| expandTrigger="hover" | |
| options={[ | |
| { | |
| label: 'Women Clothing', | |
| value: '1', | |
| children: [ | |
| { | |
| label: 'Women Tops, Blouses & Tee', | |
| value: '11', | |
| children: [ | |
| { | |
| label: 'Women T-Shirts', | |
| value: '111', | |
| }, | |
| { | |
| label: 'Women Tops', | |
| value: '112', | |
| }, | |
| { | |
| label: 'Women Tank Tops & Camis', | |
| value: '113', | |
| }, | |
| { | |
| label: 'Women Blouses', | |
| value: '114', | |
| }, | |
| ], | |
| }, | |
| { | |
| label: 'Women Suits', | |
| value: '2', | |
| children: [ | |
| { | |
| label: 'Women Suit Pants', | |
| value: '21', | |
| }, | |
| { | |
| label: 'Women Suit Sets', | |
| value: '22', | |
| }, | |
| { | |
| label: 'Women Blazers', | |
| value: '23', | |
| }, | |
| ], | |
| }, | |
| ], | |
| }, | |
| ]} | |
| showSearch | |
| checkable | |
| open | |
| />, | |
| ); | |
| fireEvent.change(wrapper.container.querySelector('input') as HTMLElement, { | |
| target: { value: 'w' }, | |
| }); | |
| const items = wrapper.container.querySelectorAll('.rc-cascader-menu-item'); | |
| fireEvent.mouseEnter(items[9]); | |
| expect(mockScrollTo).toHaveBeenCalledTimes(0); | |
| expect(items).toHaveLength(10); // 验证搜索结果的数量 | |
| expect(wrapper.container.querySelector('.rc-cascader-menu-item-active')).toBeTruthy(); // 检查是否有活动项 | |
| spyElement.mockRestore(); | |
| }); |
There was a problem hiding this comment.
@coderabbitai 不用管，只需要断言是否执行 scroll 就行。
There was a problem hiding this comment.
@afc163 明白了，我会保持测试用例的简洁性，只断言是否执行了 scrollTo。感谢您的指导！
好了 |
afc163
reviewed
Sep 3, 2024
| @@ -1022,6 +1022,85 @@ describe('Cascader.Basic', () => { | |||
| wrapper.find(`li[data-path-key]`).at(0).simulate('click'); | |||
| wrapper.find(`li[data-path-key]`).at(1).simulate('click'); | |||
| }); | |||
| it('hover + search', () => { | |||
There was a problem hiding this comment.
Suggested change
| it('hover + search', () => { | |
| it('hover + search', () => { |
16 tasks
This was referenced Oct 16, 2024
Merged
lihongxiangfrontend
pushed a commit
to lihongxiangfrontend/cascader
that referenced
this pull request
Oct 25, 2024
Co-authored-by: afc163 <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
优化为 searchValue 有值，阻止滚动，解决搜索结果选中后，没有默认激活当前选中项
优化前
iShot_2024-09-02_18.13.05.mp4
优化后
iShot_2024-09-02_18.13.35.mp4
Summary by CodeRabbit
searchValue的依赖，确保组件行为更直接。Cascader组件的测试覆盖，验证了其在特定用户交互下的行为。