Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(interactions): Press up event #5291

Closed
wants to merge 1 commit into from

Conversation

jrgarciadev
Copy link

Closes #5290

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

https://github.com/nextui-org/nextui

@jrgarciadev jrgarciadev changed the title fix(interactions): press up event fix(interactions): Press up event Oct 23, 2023
@@ -466,7 +466,7 @@ export function usePress(props: PressHookProps): PressResult {
}
} else if (state.isOverTarget) {
state.isOverTarget = false;
triggerPressEnd(createEvent(state.target, e), state.pointerType, false);
triggerPressEnd(createEvent(state.target, e), state.pointerType);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this mean we trigger onPress when the user just presses down and then drags their mouse off without releasing? I think that's what the tests are catching as well.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, here's a video, I pressed down the NextUI Button (custom usePress) and the onPress is not triggered when dragging without release, it seems to have the same behaviour.

CleanShot.2023-10-23.at.08.40.54.mp4

Here's the version with custom usePress:
https://nextui-storybook-v2-git-feat-autocomplete-nextui-org.vercel.app/?path=%2Fstory%2Fcomponents-button--with-state&vercelToolbarCode=e1L8-1_7if1HslV

Here's the version with react-aria usePress:
https://storybook.nextui.org/?path=/story/components-button--with-state

I don't see any difference

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what's going on in nextui, I see it behaving as you described. But I just tried out this branch locally with our Button story in storybook and press action is fired if I mouse down and drag off the button.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you do a diff between your version of usePress and the one in this PR to see what the difference is?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, I did a diff of the file you linked to and our code. I do not see the change you're proposing in this PR in that diff...
From what I can tell, you meant to put it one more function down, in onPointerUp?

@snowystinger
Copy link
Member

snowystinger commented Feb 12, 2024

Hey @jrgarciadev! Just checking in if you still need this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[usePress] Hook doesn't call the onPress function when pressing the element at the corner
3 participants