Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

[Core] Invoke Clicked event tapping SwipeItems #11298

Merged
merged 4 commits into from
Aug 17, 2020
Merged

[Core] Invoke Clicked event tapping SwipeItems #11298

merged 4 commits into from
Aug 17, 2020

Conversation

jsuarezruiz
Copy link
Contributor

Description of Change

SwipeItem has its own Invoked event but inherits from IMenuItem. Inheriting from IMenuItem it also has a Clicked event that was not being invoked when tapping the SwipeItem. This PR fixes this issue.

Issues Resolved

API Changes

None

Platforms Affected

  • Core/XAML (all platforms)

Behavioral/Visual Changes

None

Before/After Screenshots

fix11286

Testing Procedure

Launch Core Gallery and navigate to the issue 11286. Swipe to the right and tap the SwipeItem. If the text below the SwipeView is updated, the test has passed.

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

Copy link
Member

@StephaneDelcroix StephaneDelcroix left a comment

Choose a reason for hiding this comment

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

it's very disturbing to have different patterns for event handlers and commands, depending on the type.

MenuItem.OnClicked DOES NOT fire the Command, but SwipeItem.OnInvoked does it.

I would say that a OnXXXd method should trigger both the Command and the event (otherwise, hey, what's the point?), but I might miss something, or this could be a breaking change...

@@ -29,6 +29,7 @@ public void OnInvoked()
if (Command != null && Command.CanExecute(CommandParameter))
Copy link
Member

Choose a reason for hiding this comment

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

while you're at it, OnInvoked could be an explicit interface implementation

@samhouts samhouts changed the base branch from 4.7.0 to 4.8.0 July 8, 2020 17:10
@samhouts samhouts requested review from StephaneDelcroix and removed request for StephaneDelcroix July 30, 2020 02:30
@samhouts samhouts added this to the 5.0.0 milestone Aug 4, 2020
@samhouts samhouts added the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label Aug 4, 2020
@samhouts samhouts changed the base branch from 4.8.0 to 5.0.0 August 4, 2020 22:23
@samhouts samhouts removed the retarget-branch-required PR or associated issues target a milestone. Please target this PR to the matching branch. label Aug 4, 2020
@samhouts samhouts merged commit a35e549 into 5.0.0 Aug 17, 2020
@samhouts samhouts deleted the fix-11286 branch August 17, 2020 20:46
myroot pushed a commit to myroot/Xamarin.Forms that referenced this pull request Aug 19, 2020
Co-authored-by: Samantha Houts <samhouts@users.noreply.github.com>
fixes xamarin#11286
sung-su pushed a commit to sung-su/Xamarin.Forms that referenced this pull request Aug 20, 2020
Co-authored-by: Samantha Houts <samhouts@users.noreply.github.com>
fixes xamarin#11286
sung-su pushed a commit to sung-su/Xamarin.Forms that referenced this pull request Aug 20, 2020
Co-authored-by: Samantha Houts <samhouts@users.noreply.github.com>
fixes xamarin#11286
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants