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

Tap to move through slides #288

Merged
merged 5 commits into from Mar 26, 2020
Merged

Conversation

@theswerd
Copy link
Collaborator

theswerd commented Mar 25, 2020

What does this PR accomplish?

This will make it so when a user taps on a slide, that it moves to the next one.

Did you add any dependencies?

Nope

How did you test the change?

iOS simulator

@theswerd theswerd requested review from patniemeyer, advayDev1 and hspinks Mar 25, 2020
Copy link
Contributor

jasonTelanoff left a comment

Looks good!

@advayDev1 advayDev1 added blocked and removed blocked labels Mar 25, 2020
@patniemeyer

This comment has been minimized.

Copy link
Collaborator

patniemeyer commented Mar 25, 2020

@theswerd This is good but the elastic curve causes the card to do a little dance before it starts moving. I suggest curve: Curves.easeOut maybe? Also when you reach the last card we should ignore the tap (currently pushes the card off the screen briefly). Other than those looks great!

@patniemeyer

This comment has been minimized.

Copy link
Collaborator

patniemeyer commented Mar 25, 2020

@theswerd Maybe Curve.easeInOutCubic is better... a bit faster.

@theswerd

This comment has been minimized.

Copy link
Collaborator Author

theswerd commented Mar 26, 2020

@patniemeyer done!

@sstur
sstur approved these changes Mar 26, 2020
Copy link
Collaborator

sstur left a comment

💯🚀

@@ -63,6 +68,10 @@ class CarouselView extends StatelessWidget {
padding: EdgeInsets.only(bottom: 20),
child: pageViewIndicator(context)),
),
GestureDetector(
onTap: ()=>pageController.page == this.items.length-1?null:pageController.nextPage(duration: Duration(milliseconds: 500), curve: Curves.easeInOutCubic)

This comment has been minimized.

Copy link
@sstur

sstur Mar 26, 2020

Collaborator

Nit: Is the spacing around operators here intentional? Should we incorporate dartfmt in our pipeline to standardize style?

This comment has been minimized.

Copy link
@theswerd

theswerd Mar 26, 2020

Author Collaborator

When I wrote this my IDE wasn't working, I will re push after formatting

This comment has been minimized.

Copy link
@theswerd

theswerd Mar 26, 2020

Author Collaborator

@sstur thx, just fixed

This comment has been minimized.

Copy link
@creativecreatorormaybenot
Copy link
Collaborator

patniemeyer left a comment

Looks great. Thanks!

@patniemeyer patniemeyer merged commit e273c6a into WorldHealthOrganization:master Mar 26, 2020
5 checks passed
5 checks passed
Header rules - who-app-preview Deploy canceled
Details
Mixed content - who-app-preview Deploy canceled
Details
Pages changed - who-app-preview Deploy canceled
Details
Redirect rules - who-app-preview Deploy canceled
Details
netlify/who-app-preview/deploy-preview Deploy preview canceled.
Details
PieterAelse pushed a commit to PieterAelse/app that referenced this pull request Mar 27, 2020
* added on tap

* Got rid of page scaffold unused gesture detector

* Made requrested . changes

* Updated code styling
PieterAelse pushed a commit to PieterAelse/app that referenced this pull request Mar 27, 2020
* added on tap

* Got rid of page scaffold unused gesture detector

* Made requrested . changes

* Updated code styling
PieterAelse pushed a commit to PieterAelse/app that referenced this pull request Mar 28, 2020
* added on tap

* Got rid of page scaffold unused gesture detector

* Made requrested . changes

* Updated code styling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.