Conversation
0afa60f to
bdc1809
Compare
bdc1809 to
f59690d
Compare
|
Hi 👋 Thanks for the contribution. I've recently tightened up the contribution guidelines for FossFLOW, including enforcing Conventional Commits for PR titles. Your PR title doesn't follow the required format: Valid types: Examples:
I'm closing this PR. If your changes are still relevant, please:
Cheers, |
|
^^ apologies @Chuwee this got caught up in my purge. |
|
Hi @Chuwee I've been moving house, so I've been really busy. |
|
@stan-smith no worries! take your time |
|
@stan-smith yo? |
|
@Chuwee Im so sorry, I am just swamped at the moment, I'll try to give it a look today |
stan-smith
left a comment
There was a problem hiding this comment.
Few things to sort before this can go in:
Bug: stale closure on uiState
On master, onScroll reads fresh state every invocation:
tsconst onScroll = (e: WheelEvent) => {
const uiState = uiStateApi.getState();
// ...
Your version captures uiState.zoomSettings.trackpadMode from the outer closure, so if someone toggles trackpad mode in settings it won't actually take effect until the effect re-runs. Call uiStateApi.getState() at the top of onScroll and you're sorted.
Perf: passive: false applied unconditionally
diff- uiState.rendererEl?.addEventListener('wheel', onScroll, { passive: true });
- uiState.rendererEl?.addEventListener('wheel', onScroll, { passive: false });
This blocks the compositor thread on every wheel event for everyone, regardless of whether trackpad mode is on. preventDefault() is only relevant in trackpad mode, so passive should be conditional on the current setting — you'll need to tear down and re-register the listener when it changes.
Code duplication
The zoom-to-cursor maths (~30 lines) is copy-pasted across both the trackpad-pinch and mouse-mode branches. Pull it out into a shared helper:
tsconst applyZoom = (e: WheelEvent, oldZoom: number, newZoom: number, uiState: ...) => {
if (uiState.zoomSettings.zoomToCursor && rendererRef.current && rendererSize) {
// zoom-to-cursor maths...
} else {
uiState.actions.setZoom(newZoom);
}
};
Both branches just call applyZoom(e, oldZoom, newZoom, uiState) and you're done.
Nitpicks:
const panSpeed = 1.0 — multiplying by 1.0 is a no-op. Either drop it or make it a configurable setting.
Please rebase on current master (1.10.8) so we don't get the stale package-lock.json version diff.
There was a problem hiding this comment.
Can you rebase to 1.10.8 please
I decided to attempt to solve the issue
#220
I implemented a little trackpad mode which takes care of the problem (Settings -> Zoom)
Here's a video
Grabacion.de.pantalla.2026-02-04.a.las.11.30.06.mov