feat: add wheel event handling and corresponding tests for x-foldview-ng#2145
Conversation
🦋 Changeset detectedLatest commit: c4761b4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughAdds non-passive wheel handling and centralized scroll-delta coordination to x-foldview-ng; introduces two HTML test fixtures and a Playwright test suite validating parent/child wheel scrolling behaviors. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewSlotNgTouchEventsHandler.ts`:
- Around line 178-188: The code currently updates the parent scroll twice by
assigning parentElement.scrollTop and then calling parentElement.scrollBy,
causing double-scrolling; modify the handler in
XFoldviewSlotNgTouchEventsHandler to only perform one action: if smoothParent is
true, calculate appliedDelta using this.#parentScrollTop and previousScrollTop
but do not set parentElement.scrollTop—use parentElement.scrollBy({ top:
appliedDelta, behavior: 'smooth' }) and update this.#parentScrollTop after the
smooth call; if smoothParent is false, set parentElement.scrollTop directly
using this.#parentScrollTop (and update this.#parentScrollTop accordingly) and
remove the redundant scrollBy call, keeping references to this.#parentScrollTop,
previousScrollTop, appliedDelta and smoothParent to locate the code.
🧹 Nitpick comments (4)
packages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewSlotNgTouchEventsHandler.ts (1)
109-113: Redundant null check.
this.#elementsis always truthy after assignment on line 107, making theifcheck unnecessary.♻️ Suggested simplification
- if (this.#elements) { - for (const element of this.#elements) { - this.#childrenElemsntsScrollTop.set(element, element.scrollTop); - } + for (const element of this.#elements) { + this.#childrenElemsntsScrollTop.set(element, element.scrollTop); }packages/web-platform/web-elements/tests/x-foldview-ng-wheel.spec.ts (3)
7-11: Consider using Playwright's built-in waiting utilities.Hardcoded
setTimeoutdelays can cause test flakiness—tests may fail on slower CI machines or pass prematurely. Consider using Playwright'swaitForFunctionorexpect(...).poll()for more reliable assertions.♻️ Example using polling assertion
// Instead of: await page.mouse.wheel(0, 120); await wait(200); expect(await foldview.evaluate((dom: HTMLElement) => dom.scrollTop)).toBeGreaterThan(foldviewInitial); // Consider: await page.mouse.wheel(0, 120); await expect.poll( () => foldview.evaluate((dom: HTMLElement) => dom.scrollTop), { timeout: 2000 } ).toBeGreaterThan(foldviewInitial);
26-28: Redundant locator.
scrollviewalready references#inner-scroll(line 27), so line 28 can reuse it.♻️ Suggested simplification
const foldview = page.locator('#foldview'); const scrollview = page.locator('#inner-scroll'); - await page.locator('#inner-scroll').hover(); + await scrollview.hover();
62-64: Same redundant locator pattern.Reuse the
scrollviewvariable for the hover call.♻️ Suggested simplification
const foldview = page.locator('#foldview'); const scrollview = page.locator('#inner-scroll'); - await page.locator('#inner-scroll').hover(); + await scrollview.hover();
...ages/web-platform/web-elements/src/elements/XFoldViewNg/XFoldviewSlotNgTouchEventsHandler.ts
Outdated
Show resolved
Hide resolved
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Merging this PR will not alter performance
Comparing Footnotes
|
Web Explorer#7378 Bundle Size — 384.73KiB (+0.13%).c4761b4(current) vs 5e7e43c main#7374(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch PupilTong:p/hw/foldview-support-... Project dashboard Generated by RelativeCI Documentation Report issue |
… for smoother parent scrolling
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @lynx-js/motion@0.0.2 ### Patch Changes - Add initial support for `@lynx-js/motion` ([#1062](#1062)) ## @lynx-js/react@0.116.2 ### Patch Changes - Fix "TypeError: not a function" error caused by `replaceAll` not supported in ES5. ([#2142](#2142)) - Bump `swc_core` v56. ([#2154](#2154)) - Use `disableDeprecatedWarning` option to suppress BROKEN warnings during compilation. ([#2157](#2157)) 1. BROKEN: `getNodeRef`/`getNodeRefFromRoot`/`createSelectorQuery` on component instance is broken and MUST be migrated in ReactLynx 3.0, please use ref or lynx.createSelectorQuery instead. 2. BROKEN: `getElementById` on component instance is broken and MUST be migrated in ReactLynx 3.0, please use ref or lynx.getElementById instead. - Fix memory leak by clearing list callbacks when \_\_DestroyLifetime event is triggered. ([#2112](#2112)) ## @lynx-js/rspeedy@0.13.3 ### Patch Changes - Updated dependencies \[]: - @lynx-js/web-rsbuild-server-middleware@0.19.7 ## @lynx-js/qrcode-rsbuild-plugin@0.4.4 ### Patch Changes - Bump `@clack/prompts` v1.0 ([#2171](#2171)) ## @lynx-js/react-rsbuild-plugin@0.12.7 ### Patch Changes - Updated dependencies \[[`92881e7`](92881e7), [`1a5f2a1`](1a5f2a1)]: - @lynx-js/template-webpack-plugin@0.10.3 - @lynx-js/css-extract-webpack-plugin@0.7.0 - @lynx-js/react-webpack-plugin@0.7.3 - @lynx-js/react-alias-rsbuild-plugin@0.12.7 - @lynx-js/use-sync-external-store@1.5.0 - @lynx-js/react-refresh-webpack-plugin@0.3.4 ## @lynx-js/web-constants@0.19.7 ### Patch Changes - Updated dependencies \[]: - @lynx-js/web-worker-rpc@0.19.7 ## @lynx-js/web-core@0.19.7 ### Patch Changes - feat: add browser config of lynx-view, now you can customize the browser config of lynx-view: ([#2140](#2140)) lynxView.browserConfig = { pixelRatio: 1, pixelWidth: 1234, pixelHeight: 5678, } - Updated dependencies \[]: - @lynx-js/web-constants@0.19.7 - @lynx-js/web-mainthread-apis@0.19.7 - @lynx-js/web-worker-rpc@0.19.7 - @lynx-js/web-worker-runtime@0.19.7 ## @lynx-js/web-core-wasm@0.0.2 ### Patch Changes - Updated dependencies \[[`43fc7e7`](43fc7e7)]: - @lynx-js/web-elements@0.11.1 - @lynx-js/web-worker-rpc@0.19.7 ## @lynx-js/web-elements@0.11.1 ### Patch Changes - feat: add wheel event handling and corresponding tests for x-foldview-ng ([#2145](#2145)) ## @lynx-js/web-mainthread-apis@0.19.7 ### Patch Changes - Updated dependencies \[]: - @lynx-js/web-constants@0.19.7 ## @lynx-js/web-worker-runtime@0.19.7 ### Patch Changes - Updated dependencies \[]: - @lynx-js/web-constants@0.19.7 - @lynx-js/web-mainthread-apis@0.19.7 - @lynx-js/web-worker-rpc@0.19.7 ## @lynx-js/template-webpack-plugin@0.10.3 ### Patch Changes - Fix "Failed to load CSS update file" for lazy bundle ([#2150](#2150)) - Fix "TypeError: cannot read property 'call' of undefined" error of lazy bundle HMR. ([#2146](#2146)) ## create-rspeedy@0.13.3 ## @lynx-js/react-alias-rsbuild-plugin@0.12.7 ## upgrade-rspeedy@0.13.3 ## @lynx-js/web-core-server@0.19.7 ## @lynx-js/web-rsbuild-server-middleware@0.19.7 ## @lynx-js/web-worker-rpc@0.19.7 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Checklist