takaebato commented on code in PR #21588:
URL: https://github.com/apache/echarts/pull/21588#discussion_r3143524245
##########
src/component/helper/RoamController.ts:
##########
@@ -362,43 +392,43 @@ class RoamController extends
Eventful<RoamEventDefinition> {
const shouldZoom = isAvailableBehavior('zoomOnMouseWheel', e,
this._opt);
const shouldMove = isAvailableBehavior('moveOnMouseWheel', e,
this._opt);
const wheelDelta = e.wheelDelta;
- const absWheelDeltaDelta = Math.abs(wheelDelta);
- const originX = e.offsetX;
- const originY = e.offsetY;
-
// wheelDelta maybe -0 in chrome mac.
if (wheelDelta === 0 || (!shouldZoom && !shouldMove)) {
return;
}
- // If both `shouldZoom` and `shouldMove` is true, trigger
- // their event both, and the final behavior is determined
- // by event listener themselves.
+ // Reach through to the native WheelEvent for per-axis deltas;
+ // zrender's `wheelDelta` collapses the two axes into a single scalar.
+ // Pre-2013 IE has no `deltaY`; the axis helpers fall back to the
+ // passed default in that case.
+ const nativeEvent = (e.event as unknown as WheelEvent) || null;
+ const originX = e.offsetX;
+ const originY = e.offsetY;
if (shouldZoom) {
- // Convenience:
- // Mac and VM Windows on Mac: scroll up: zoom out.
- // Windows: scroll up: zoom in.
-
- // FIXME: Should do more test in different environment.
- // wheelDelta is too complicated in difference nvironment
- //
(https://developer.mozilla.org/en-US/docs/Web/Events/mousewheel),
- // although it has been normallized by zrender.
- // wheelDelta of mouse wheel is bigger than touch pad.
- const factor = absWheelDeltaDelta > 3 ? 1.4 : absWheelDeltaDelta >
1 ? 1.2 : 1.1;
- const scale = wheelDelta > 0 ? factor : 1 / factor;
+ // `scaleY` defaults to `scale` on the IE fallback so the single
+ // scalar still drives zoom; `scaleX` defaults to identity so
+ // `zoomOnMouseWheelAxis: 'horizontal'` becomes a safe no-op.
+ const scale = ladderZoomScale(wheelDelta);
this._checkTriggerMoveZoom(this, 'zoom', 'zoomOnMouseWheel', e, {
- scale: scale, originX: originX, originY: originY,
isAvailableBehavior: null
+ scale: scale,
+ scaleX: axisZoomScale(nativeEvent, 'horizontal', 1),
Review Comment:
A separate API design question: would it make sense to let callers express
the "no restriction" state while keeping the field present?
There seem to be a couple of independent dimensions to consider:
1. Whether to add an explicit literal (e.g. `'all'`, or `'both'`)
2. Whether to admit `null` / `undefined` in the type
Some existing precedents in echarts I found:
- `NullUndefined` only
-
https://github.com/apache/echarts/blob/c8adaf4f509d217d55cab96a4a9535b431d97fb4/src/util/types.ts#L1081
- both
-
https://github.com/apache/echarts/blob/c8adaf4f509d217d55cab96a4a9535b431d97fb4/src/coord/cartesian/GridModel.ts#L65
-
https://github.com/apache/echarts/blob/c8adaf4f509d217d55cab96a4a9535b431d97fb4/src/util/model.ts#L770
So some candidate shapes might be:
```ts
// (a) literal only
moveOnMouseWheelAxis?: 'horizontal' | 'vertical' | 'all'
// (b) NullUndefined only
moveOnMouseWheelAxis?: 'horizontal' | 'vertical' | NullUndefined
// (c) both (matches outerBoundsMode / ModelFinderIndexQuery shape)
moveOnMouseWheelAxis?: 'horizontal' | 'vertical' | 'all' | NullUndefined
// (d) keep as-is
moveOnMouseWheelAxis?: 'horizontal' | 'vertical'
```
(b) keeps the API surface minimal while still letting callers express the
no-restriction state. That said, if there's no clear demand yet, sticking with
the current (d) might align fine with the rest of the codebase. Happy with
whichever direction.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
