Conversation
robyww
added
bug
enhancement
UI
robyww
force-pushed
the
FIREFLY-1947-ipad
branch
from
4873e18 to
5760e05
Compare
jaladh-singhal
left a comment
jaladh-singhal
left a comment
There was a problem hiding this comment.
Tested all mentioned things - working great!
Code looks good, left some minor comments for DRY.
I did notice one issue though which can be tackled separately: When I turn on "click lock" in the bottom right of an image, I don't see a pink marker that comes up in desktop and mouse readout doesn't update. Since you already implemented tap to see readout which acts as click lock on touch devices, maybe we can hide the "click lock" slider when a device is touch screen?
| } | ||
| }; | ||
|
|
||
| const onTouchMove= (ev) => { |
There was a problem hiding this comment.
Wow, these are throughly thought out!
| eRef.element?.addEventListener('wheel', onWheel, {passive:false}); | ||
| return () => eRef.element?.removeEventListener('wheel', onWheel); | ||
| }, [eRef.element, transform, plotId, eventCallback]); | ||
| eRef.element?.addEventListener('touchmove', onTouchMove, {passive:false}); | ||
| eRef.element?.addEventListener('touchstart', onTouchStart, {passive:false}); | ||
| eRef.element?.addEventListener('touchend', onTouchEnd, {passive:false}); | ||
| eRef.element?.addEventListener('touchcancel', onTouchCancel, {passive:false}); |
There was a problem hiding this comment.
[Nitpick] Could have declared the event and callback as a list of list (const events = [[event, callback], ...]) and called addEventListener only once as events.forEach().
There was a problem hiding this comment.
good idea, I will do that
| eRef.element?.removeEventListener('wheel', onWheel); | ||
| eRef.element?.removeEventListener('touchmove', onTouchMove); | ||
| eRef.element?.removeEventListener('touchstart', onTouchStart); | ||
| eRef.element?.removeEventListener('touchend', onTouchEnd); | ||
| eRef.element?.removeEventListener('touchcancel', onTouchCancel); | ||
| }; |
There was a problem hiding this comment.
same as above: events.forEach(([event callback])=>eRef.element?.removeEventListener(event, callabck)). Single events list will make sure nothing is missed from cleanup if it updates in future.
robyww
force-pushed
the
FIREFLY-1947-ipad
branch
from
5760e05 to
a21e5dd
Compare
- Make initial size correct using dvw units - Tap on image and see readout - Pinch to zoom - Support touch events with overlays - improved menubar size test and fix display mask - includes reponse to feedback
robyww
force-pushed
the
FIREFLY-1947-ipad
branch
from
a21e5dd to
7c090a0
Compare
2 participants
Firefly-1947: Improve firefly on iPad
This is a start of some iPad work. So the scope is pretty narrow. It is just addressing some of the bigger issues. There will be more do to that we will address in future tickets.
dvwunitstesting