Skip to content

MudSwipeArea: SwipeEventArgs Feature#6574

Merged
henon merged 5 commits intoMudBlazor:devfrom
mckaragoz:MudSwipeAreaSwipeEventArgs
Apr 4, 2023
Merged

MudSwipeArea: SwipeEventArgs Feature#6574
henon merged 5 commits intoMudBlazor:devfrom
mckaragoz:MudSwipeAreaSwipeEventArgs

Conversation

@mckaragoz
Copy link
Member

@mckaragoz mckaragoz commented Mar 31, 2023

Description

This PR aims to handle OnSwipe properly.

  1. Firstly changed OnSwipe Obsolete and added OnSwipeEnd. OnSwipe is an Action<SwipeDirection>, OnSwipeEnd is EventCallback<SwipeEventArgs>
  2. Added SwipeEventArgs class to reach swipe direction, swipe delta, touch event args and MudSwipeArea as sender.
  3. Change SwipeDelta determination time to have consistent result.

Its not a breaking change, just obsoleted OnSwipe and GetSwipeDelta method.

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests.

@github-actions github-actions bot added enhancement Adds a new feature or enhances existing functionality (not fixing a defect) in the main library PR: needs review labels Mar 31, 2023
@codecov
Copy link

codecov bot commented Mar 31, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.05 🎉

Comparison is base (b53b348) 91.37% compared to head (6a0e406) 91.42%.

Additional details and impacted files 
@@            Coverage Diff             @@
##              dev    #6574      +/-   ##
==========================================
+ Coverage   91.37%   91.42%   +0.05%     
==========================================
  Files         397      398       +1     
  Lines       14916    14959      +43     
==========================================
+ Hits        13629    13677      +48     
+ Misses       1287     1282       -5
Impacted Files Coverage Δ
...dBlazor/Components/SwipeArea/MudSwipeArea.razor.cs 100.00% <100.00%> (+15.00%) ⬆️
...c/MudBlazor/Components/SwipeArea/SwipeEventArgs.cs 100.00% <100.00%> (ø)

... and 16 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

henon
henon requested changes Mar 31, 2023
View reviewed changes
Copy link
Contributor

@henon henon left a comment

Choose a reason for hiding this comment

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

I like the change. Test coverage is low though.

@ScarletKuro
Copy link
Member

ScarletKuro commented Mar 31, 2023 •
edited
Loading

I also like the change, EventCallback is much better than Action for parameter, and it supports Task.
But can we have SwipeEventArgs in it's own file SwipeEventArgs.cs, I think it's a good practice.

ScarletKuro
ScarletKuro reviewed Mar 31, 2023
View reviewed changes
src/MudBlazor/Components/SwipeArea/MudSwipeArea.razor.cs Outdated
}

internal void OnTouchEnd(TouchEventArgs arg)
internal async void OnTouchEnd(TouchEventArgs arg)
Copy link
Member

@ScarletKuro ScarletKuro Mar 31, 2023

Choose a reason for hiding this comment

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

Now when it's async can we change void to Task. And since it's internal rename to OnTouchEndAsync

Copy link
Member Author

@mckaragoz mckaragoz Mar 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Im not sure we add async suffix even on async Tasks. @henon? But i agree with change void to Task.

Copy link
Contributor

@henon henon Apr 1, 2023

Choose a reason for hiding this comment

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

Up until now we have not had a rule that we have to have an ...Async postfix in private functions or event handlers if they are async. We try to have the postfix in public API though (i.e. FocusAsync etc) even though there may be some inconsistencies as we didn't do that in the beginning.

@mckaragoz
Copy link
Member Author

mckaragoz commented Mar 31, 2023

I also like the change, EventCallback is much better than Action for parameter, and it supports Task. But can we have SwipeEventArgs in it's own file SwipeEventArgs.cs, I think it's a good practice.

Completely agree, only i didn't decide where to add this file.

@ScarletKuro
Copy link
Member

ScarletKuro commented Mar 31, 2023 •
edited
Loading

Completely agree, only i didn't decide where to add this file.

It's totally fine to have it in the SwipeArea folder. Other components like Tabs, Table etc. have the EventArgs in their component folder. And there is not much choice anyway, since we don't have any "Event" folder.

@henon henon added needs: tests A maintainer has explicitly asked for associated test cases to be added to this pull request and removed PR: needs review labels Apr 1, 2023
@mckaragoz
Copy link
Member Author

mckaragoz commented Apr 2, 2023

Changed the requested things and add test, now it coverage %100.

@ScarletKuro
Copy link
Member

ScarletKuro commented Apr 2, 2023

I would maybe consider SwipeEventArgs making read-only, i.e. making only get properties and values set through constructor, would be easier for nullable too in future, but it's really optional.
So far LGTM

@henon
Copy link
Contributor

henon commented Apr 4, 2023

@mckaragoz I think @ScarletKuro is right, it would be better to make all setters readonly and private so they can only be set via the constructor. We can't change this later as it is public API so let's do it now, OK?

@mckaragoz
Copy link
Member Author

mckaragoz commented Apr 4, 2023

Changed, if it's proper, we can merge

@henon henon merged commit 536ca9b into MudBlazor:dev Apr 4, 2023
@henon henon removed the needs: tests A maintainer has explicitly asked for associated test cases to be added to this pull request label Apr 4, 2023
ilovepilav pushed a commit to ilovepilav/MudBlazor that referenced this pull request Nov 25, 2023
@mckaragoz @ilovepilav
MudSwipeArea: SwipeEventArgs Feature (MudBlazor#6574)
826b54d 
* MudSwipeArea SwipeEventArgs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@henon henon henon requested changes

@ScarletKuro ScarletKuro ScarletKuro approved these changes

Assignees

No one assigned

Labels

enhancement Adds a new feature or enhances existing functionality (not fixing a defect) in the main library

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mckaragoz @ScarletKuro @henon