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

feat: Add onReleased action to AdvancedButtonComponent which will be called within onTapUp #3152

Merged
merged 2 commits into from
May 10, 2024

Conversation

mabdelaal86
Copy link
Contributor

@mabdelaal86 mabdelaal86 commented May 8, 2024

Description

Add onReleased action which will be called within onTapUp. That fixed some scenarios which require the pressed action to be called after the user taps up the button.

For example, AdvancedButtonComponent doesn't show the downSkin if the action of onPressed changes the router to another page. That is because the router is changed before the downSkin is shown as the onPressed action is called in onTapDown.

Checklist

  • I have followed the Contributor Guide when preparing my PR.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples or docs.

Breaking Change?

  • Yes, this PR is a breaking change.
  • No, this PR is not a breaking change.

Related Issues

Fixes #3150

…gine#3150)

`AdvancedButtonComponent` doesn't show the `downSkin` if the action of `onPressed` changes the router to another page. That is because the router is changed before the `downSkin` is shown as the `onPressed` action is called in `onTapDown` instead of `onTapUp`.

In this PR, calling `onPressed` action is moved to `onTapUp` method instead of `onTapDown`. That makes the `AdvancedButtonComponent` more natural and allows users to skip the action if they move the mouse pointer or their finger outside the button box.
@mabdelaal86 mabdelaal86 marked this pull request as ready for review May 8, 2024 14:58
@mabdelaal86
Copy link
Contributor Author

The failed test case seems unrelated to my changes and all tests are passed locally with melos run test!
Is there a way to rerun the checks again?

@spydon
Copy link
Member

spydon commented May 9, 2024

I don't think this is worth a breaking change, it's better to expose a callback for onTapUp. In UI's buttons often trigger in onTapUp, but in games, when mashing a fire button for example, you usually want onTapDown.

@mabdelaal86
Copy link
Contributor Author

Yeah, that makes sense. I didn't think about it, since I'm developing mainly board games, which are kind of UI apps.

@mabdelaal86
Copy link
Contributor Author

Does it worth adding a callback to the AdvancedBottonComponent for onTapUp? or just implement a custom version in my game?

@spydon
Copy link
Member

spydon commented May 9, 2024

Does it worth adding a callback to the AdvancedBottonComponent for onTapUp? or just implement a custom version in my game?

I think it would be worth adding! I guess we should add onTapDown then too and deprecate onPressed in the long run.

@mabdelaal86
Copy link
Contributor Author

I have added onReleased event which will be called from onTapUp.

@mabdelaal86 mabdelaal86 changed the title fix: Make AdvancedButtonComponent call onPressed in onTapUp instead of onTapDown feat: Add onReleased action to AdvancedButtonComponent which will be called within onTapUp May 10, 2024
@spydon
Copy link
Member

spydon commented May 10, 2024

I have added onReleased event which will be called from onTapUp.

Sounds good! Then we can refactor it to onTapDown and onTapUp for V2 instead (in a distant future)

Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

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

Lgtm, thanks for your contribution!

@spydon spydon merged commit 2269732 into flame-engine:main May 10, 2024
10 checks passed
@mabdelaal86
Copy link
Contributor Author

Thanks a lot for the fast review

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.

AdvancedButtonComponent doesn't show downSkin when changing the route
2 participants