Justin-ZS commented on code in PR #21588:
URL: https://github.com/apache/echarts/pull/21588#discussion_r3145350951
##########
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 small refinement to what I said above: maybe “clear” is not the best
framing here. If `null` and `undefined` would be treated the same, then for
hand-written options, simply omitting the field already expresses the default
no-restriction behavior clearly.
So I’m not sure we need to widen the public option type in this PR.
`NullUndefined` would mainly help generated/config-form scenarios where the
field is always present, or update flows that need a field-present way to go
back to the default state. If we do not have that concrete need yet, keeping
the API as `moveOnMouseWheelAxis?: 'horizontal' | 'vertical'` /
`zoomOnMouseWheelAxis?: 'horizontal' | 'vertical'` seems simpler.
I’d lean toward keeping the API as-is for now, and only adding
`NullUndefined` or a literal like `'all'` later if a real use case needs an
explicit field-present default/no-restriction representation.
##########
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:
Thinking about this again, I’d keep the API as-is for now. If `null` and
`undefined` would behave the same here, then omitting the field already clearly
expresses the default no-restriction behavior for hand-written options.
`NullUndefined` or a literal like `'all'` would mainly be useful if we have
a concrete need for a field-present default/no-restriction representation, such
as generated configs or update flows. Without that need in this PR,
`moveOnMouseWheelAxis?: 'horizontal' | 'vertical'` / `zoomOnMouseWheelAxis?:
'horizontal' | 'vertical'` seems simpler.
--
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]
