Feat/event coordinates #6482
Feat/event coordinates #6482
Conversation
|
Thanks for taking this on! It looks like some of the tests are failing. |
|
Yes but I've noticed the failing tests appear unrelated to my changes. What could be causing this? |
|
No worries, it looks like there are console logs left in the code, that would cause test failures |
|
GET_BUILD |
There was a problem hiding this comment.
Thanks for fixing the tests
I see we still need to calculate the coordinates relative to the target
we'd like for coordinates to be relative to the target element. For any event that doesn't contain location information, such as keyboard, the center of the target element should be used for the x and y
It may be helpful to implement a storybook example showing how it'd be used.
Something along these lines maybe
Button.stories.tsx
function RippleButton(props) {
const [coords, setCoords] = useState({x: -1, y: -1});
const [isRippling, setIsRippling] = useState(false);
useEffect(() => {
if (coords.x !== -1 && coords.y !== -1) {
setIsRippling(true);
setTimeout(() => setIsRippling(false), 300);
} else {
setIsRippling(false);
}
}, [coords]);
useEffect(() => {
if (!isRippling) {
setCoords({x: -1, y: -1});
}
}, [isRippling]);
let onPress = (e) => {
console.log(e);
setCoords({x: e.x, y: e.y});
};
console.log(coords)
return (
<Button {...mergeProps(props, {onPress})} className={styles['ripple-button']}>
{isRippling ? (
<span
className={styles['ripple']}
style={{
left: coords.x,
top: coords.y
}} />
) : (
''
)}
<span className="content">{props.children}</span>
</Button>
);
}
button-ripple.css
.ripple-button {
position: relative;
border-radius: 4px;
border: none;
margin: 8px;
padding: 14px 24px;
background: #1976d2;
color: #fff;
overflow: hidden;
position: relative;
cursor: pointer;
}
.ripple-button > .ripple {
width: 20px;
height: 20px;
position: absolute;
background: #63a4ff;
display: block;
content: "";
border-radius: 9999px;
opacity: 1;
animation: 0.9s ease 1 forwards ripple-effect;
}
@keyframes ripple-effect {
0% {
transform: scale(1);
opacity: 1;
}
50% {
transform: scale(10);
opacity: 0.375;
}
100% {
transform: scale(35);
opacity: 0;
}
}
.ripple-button > .content {
position: relative;
z-index: 2;
}
|
Yes sure I will implement an example. Just for my understanding the center of the target should be x: 0 and y: 0? |
|
coordinates should be relative to the top left of the element. so top left is 0, 0. |
|
Thanks for the prompt update. I added a couple tests and a story. We'll need to address the keyboard so the ripple appears in the middle as expected. In addition, I think virtual clicks from Assistive Technologies such as VoiceOver has been left out, we'll want to include that as well. I think a few |
|
Ok thank you for adding this. I will try to include the calculation for the missing x and y coordinates. 🙂 |
|
Hey everything should work now as expected. I dont know if the solution I came up with suits you. Just let me know if there is anything that should be improved. |
| let stopPressStart = triggerPressStart(e, 'virtual'); | ||
| let stopPressUp = triggerPressUp(e, 'virtual'); | ||
| let stopPressEnd = triggerPressEnd(e, 'virtual'); | ||
| let stopPressStart = triggerPressStart(createEvent(state.target, initEventCoordinates(e, state.target)), 'virtual'); |
There was a problem hiding this comment.
looks like this has introduced a bug, that's why there are some failing tests, you don't appear to be passing through the event.currentTarget anymore
so in triggerPressStart, it throws an error when the new PressEvent is created and we try to read the getBoundingClientRect
There was a problem hiding this comment.
If I understood you right it should be fixed now please have a look too.
| @@ -628,7 +635,7 @@ export function usePress(props: PressHookProps): PressResult { | |||
| disableTextSelection(state.target); | |||
| } | |||
|
|
|||
| let shouldStopPropagation = triggerPressStart(e, state.pointerType); | |||
| let shouldStopPropagation = triggerPressStart(initEventCoordinates(e, state.target), state.pointerType); | |||
There was a problem hiding this comment.
this is a touch event, it should already have a clientX and clientY if you go through the TouchList https://developer.mozilla.org/en-US/docs/Web/API/Touch apologies for saying a bunch of these were missed, I hadn't quite thought through it all the way, I think it was only a couple
the other touch events should also have those properties
However, maybe we don't worry about the press events which come from touchstart/touchend and mousedown/mouseup, the pointer events are supported on all major browsers, so theoretically these are never being hit except in jsdom tests where pointer events are not supported yet
I'll see how the rest of the team feels, but for now, don't worry about them.
you've found an interesting set, what to do for canceled press. it can happen for a few reasons, one of which is that the cursor is no longer on the target. i think you're doing the right thing by not treating it differently
| clientX: 0, | ||
| clientY: 0 |
There was a problem hiding this comment.
will this come through as 0,0 or will it come through as the center of the target?
There was a problem hiding this comment.
Yes it will get passed 0 0 and the triggerPressEnd inside the cancel function would then expose x: 0 - rect.left
Should I instead calculate the absolute center position inside the cancel function, so the triggerPressEnd can expose the realtive center coordinates?
There was a problem hiding this comment.
yeah, i think that the center makes more sense than 0 - rect.left
|
I am struggling a bit with the unit test Could you have a look please. |
|
Thanks for your work on this, I've pushed a few changes. The problem you were facing was we were trying to mutate an event object which wasn't allowed. I've refactored to avoid that and centralize some handling. |
|
GET_BUILD |
|
Thank you, the code is much better now, learned a lot looking at your changes. Right now I didn't came up with any new ideas for unit tests 🙂. |
|
GET_BUILD |
|
## API Changes
unknown top level export { type: 'any' } @react-aria/interactionsPressEvent PressEvent {
altKey: boolean
continuePropagation: () => void
ctrlKey: boolean
metaKey: boolean
pointerType: PointerType
shiftKey: boolean
target: Element
type: 'pressstart' | 'pressend' | 'pressup' | 'press'
+ x: number
+ y: number
}it changed:
|
Closes #2031
Related #1998.
Original PR #4267
Since the original PR is not maintained anymore, I had to create a new one.
Add clientX and clientY positioning to onPress and onHover events for implementing ripple or wave effect.
✅ Pull Request Checklist:
📝 Test Instructions: