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.

Sign up for GitHub

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

Jump to bottom

feat: add swipe dismiss sensitivity params for modals #222

Merged
merged 23 commits into from
Aug 31, 2024
Merged

feat: add swipe dismiss sensitivity params for modals #222

merged 23 commits into from
Aug 31, 2024

Conversation

ice-orion
Copy link
Contributor

@ice-orion ice-orion commented Aug 19, 2024 •
edited
Loading

Description

Thank u for the great lib!

The PR adds SwipeDismissConfig class that can be passed to CupertinoModalSheetPage, CupertinoModalSheetRoute, ModalSheetPage and ModalSheetRoute classes to configure min swipe velocity and distance for a swipe to result in a dismissal.

@ice-orion ice-orion mentioned this pull request Aug 19, 2024
Merged
ice-orion added a commit to ice-blockchain/ion-app that referenced this pull request Aug 20, 2024
@ice-orion
chore: get started fixes (#148)
99e6a67 
[Notion
task](https://www.notion.so/leftclick/Fix-get-started-e324d9a0be544b2ea35e8634820d0fdc)

This PR uses our [custom
version](fujidaiti/smooth_sheets#222) of
smooth_sheets lib. So that we could control the
`minFlingVelocityToDismiss` and `minDragDistanceToDismiss` dismiss
params.

Also this PR sets all font letter spacing to `0` (to make fonts look
exactly like in figma) - keep that in mind because that might change the
markdown in some places.
@appinteractive
Copy link

Awesome! But I think it would make more sense to add a DismissConfig (or similar) that contains the both values. At least that is how the other configs get encapsulated as far as I can tell.

@ice-orion
feat: add dismiss configuration params to modals
05edf0e 
Add minFlingVelocityToDismiss and minDragDistanceToDismiss params for CupertinoModalSheetPage, CupertinoModalSheetRoute, ModalSheetPage and ModalSheetRoute classes
@ice-orion
feat: add SwipeDismissConfig for modals
9b860b8
@ice-orion ice-orion force-pushed the feature/modal-dismiss-params branch from 2d7e899 to 9b860b8 Compare August 20, 2024 11:08
@ice-orion
Copy link
Contributor Author

Good point! @appinteractive check the changes please, added SwipeDismissConfig as u suggested

@fujidaiti fujidaiti linked an issue Aug 22, 2024 that may be closed by this pull request
Add a way to customize sensitivity of swipe-to-dismiss action on modal sheet #162
Closed
fujidaiti
fujidaiti reviewed Aug 22, 2024
View reviewed changes
Copy link
Owner

@fujidaiti fujidaiti left a comment

Choose a reason for hiding this comment

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

@ice-orion

Thank you for the contribution! It looks generally good, but there are a few points I'd like to discuss before we merge this PR.

package/lib/src/modal/swipe_dismiss_config.dart Outdated Show resolved Hide resolved
package/lib/src/modal/swipe_dismiss_config.dart Outdated Show resolved Hide resolved
@ice-orion
refactor: rename SwipeDismissConfig, remove util class methods
fdceb2c 
rename SwipeDismissConfig to SwipeDismissSensitivity
remove toString, ==, hashCode methods
@ice-orion ice-orion force-pushed the feature/modal-dismiss-params branch from 2076293 to fdceb2c Compare August 22, 2024 14:08
@fujidaiti
Copy link
Owner

Can you also make the following changes?

  • Bump the version to 0.10.0 in pubspec.yaml and update CHANGELOG file
  • Add an example of SwipeDismissSensitivity to tutorial/imperative_modal_route.dart or somewhere
  • Run dart format . to ensure the CI passes

@ice-orion ice-orion changed the title feat: add dismiss configuration params to modals feat: add swipe dismiss sensitivity params for modals Aug 23, 2024
@ice-orion
Copy link
Contributor Author

@fujidaiti Done, check please

@fujidaiti
Copy link
Owner

@ice-orion

I renamed minFlingVelocity to minFlingVelocityRatio to reflect its actual meaning and changed the default value from 1.0 to 2.0. Could you test the new default configuration to see if it's okay?

@ice-orion
Copy link
Contributor Author

@fujidaiti In our project, we even set the minFlingVelocity parameter to 3, likely because most of our modals are nearly full-height. However, for medium to shorter modals, 3 might be too high for comfortable usage. Overall, 2 seems to be the golden mean here.

@fujidaiti
Copy link
Owner

@ice-orion Thank you! I'll resolve the conflicts and add some tests this weekend.

@fujidaiti
Copy link
Owner

fujidaiti commented Aug 31, 2024 •
edited
Loading

@ice-orion I changed the default minDragDistance to 200, as 300 is a bit too large especially for small sheets.

@fujidaiti fujidaiti enabled auto-merge (squash) August 31, 2024 05:58
@fujidaiti fujidaiti disabled auto-merge August 31, 2024 05:58
@fujidaiti fujidaiti enabled auto-merge (squash) August 31, 2024 05:59
@fujidaiti fujidaiti force-pushed the feature/modal-dismiss-params branch from 414558b to 91c0be3 Compare August 31, 2024 06:00
@fujidaiti fujidaiti merged commit 9d07275 into fujidaiti:main Aug 31, 2024
5 checks passed
@ice-orion ice-orion deleted the feature/modal-dismiss-params branch September 5, 2024 10:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fujidaiti fujidaiti fujidaiti left review comments

Assignees

@ice-orion ice-orion

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a way to customize sensitivity of swipe-to-dismiss action on modal sheet
3 participants
@ice-orion @appinteractive @fujidaiti