PB-1054 : Remove mouse events from useMovableElement and replace with pointer events#1345
Conversation
web-mapviewer
|
||||||||||||||||||||||||||||
| Project |
web-mapviewer
|
| Branch Review |
bug-PB-1054-tooltip-touch-events
|
| Run status |
|
| Run duration | 06m 17s |
| Commit |
|
| Committer | Sami Nouidri |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
20
|
Skipped
|
0
|
Passing
|
251
|
| View all changes introduced in this branch ↗︎ | |
bead635 to
26912a7
Compare
26912a7 to
1a5b6de
Compare
pakb
left a comment
There was a problem hiding this comment.
Good idea, but the outcome isn't there yet.
I've tested a bit on my phone (iPhone) and I can't "scroll" the content of a layer description anymore, as it tries to move it around.
So there's maybe something to do to not prevent scrolling
2273721 to
5b61e0c
Compare
| const el = toValue(element) | ||
| const rect = el.getBoundingClientRect() | ||
| const newLeft = clamp( | ||
| el.offsetLeft + deltaX, | ||
| viewport.value.left, | ||
| viewport.value.right - rect.width | ||
| ) | ||
| const newTop = clamp( | ||
| el.offsetTop + deltaY, | ||
| viewport.value.top, | ||
| viewport.value.bottom - rect.height | ||
| ) | ||
|
|
||
| placeElementAt(newTop, newLeft, MovementSource.POINTER_DRAG) |
There was a problem hiding this comment.
except the presence of the deltaX / deltaY within the clamp, this is exactly the same as the constrainElementWithinViewport function.
I would either :
- change the
offsetLeft/offsetRightattributes from elem, then callconstrainElementWithinViewportor - add the deltaX / deltaY to the function as optional parameters (defaulting to 0) and call the function with the parameter in this way :
constrainElementWithinViewport(deltaX, deltaY)
There was a problem hiding this comment.
that being said, I might be wrong about this... it's just that I like functions ^^
There was a problem hiding this comment.
I went with the second option of adding delta arguments to constrainElementWithinViewport as offsetLeft
and offsetRight are getters only.
343c9a9 to
4e4a795
Compare
ltkum
left a comment
There was a problem hiding this comment.
A small refinement here and there, but the functionality seems good to me
| useMovableElement(popover.value, { | ||
| useMovableElement({ | ||
| element: popover.value, | ||
| grabElement: popoverHeader, |
There was a problem hiding this comment.
it seems to work, but maybe we need to be attentive to the fact that in other cases, we're using the reference's value (.value) When tackling #1309, you might want to remember this difference.
There was a problem hiding this comment.
Setting this to grabElement.value would mean that we can get rid of all the toValue in the composable and simplify it, as well as ensuring we always give the same kind of variable when calling the function. I would take advantage of the situation and do it now :)
| * @param element Reference to a DOM element | ||
| * @param options Options such as described below | ||
| * @param config {Object} Configuration object | ||
| * @param {HTMLElement} [options.grabElement] HTML element on which the user has to grab to start |
There was a problem hiding this comment.
we need to update the comment :
- it's no longer
options.keybutconfig.key. - also we need to add the
elementto the config doc here
| initialPositionClasses = [], | ||
| } = config | ||
|
|
||
| if (!config.element || !config.grabElement) { |
There was a problem hiding this comment.
offset is always defined, we should also enforce offset to have a value.
| } = config | ||
|
|
||
| if (!config.element || !config.grabElement) { | ||
| throw new Error('Element/grabElement is required') |
There was a problem hiding this comment.
I would make the error a bit more explicit :
[useMovableElement] Element, grabElement and offset are required to use a movable element. Received ${element} for element, ${grabElement} for grabElement and ${offset} for offset. or something like that.
| MovementSource.WINDOW_RESIZE | ||
| ) | ||
| } | ||
| function constrainElementWithinViewport(deltaX = 0, deltaY = 0) { |
There was a problem hiding this comment.
I would not put default values here, and add the 0, 0 as parameter in the initialize call. This forces us to be explicit in tests and in the code.
I would also rework the comment a bit, as we now also handle placing an element and not only ensuring it's within bounds at startup.
2695b72 to
c90bc5d
Compare
| element: windowElement.value, | ||
| grabElement: headerElement.value, |
There was a problem hiding this comment.
thos are already the values of windowRef or headerRef, no need for an extra value here.
…vents Mouse events have been removed in favour of pointer events for better compatibility with mobile devices and touch screens.
c90bc5d to
54ae2d8
Compare
the problem will be solved in another ticket (PB-1309)
Mouse events have been removed in favour of pointer events for better compatibility with mobile devices and touch screens.
Test link