-
Notifications
You must be signed in to change notification settings - Fork 11
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
add missing vibration on the button #137
Conversation
Small changes for new version
Fix errors in console
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Welcome to Infinite Horizons.
I have added request for change, please fix and press the re review button for me to start checking the pr again.
Future<void> _vibrateDevice() async { | ||
if (await Vibrate.canVibrate) { | ||
Vibrate.vibrate(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are using controllers for api calls.
Use VibrationController.instance.vibrate(VibrationType.light);
directly
Hey! I added new changes to it 2 days ago, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late response, not sure how did I missed it.
void onDone(BuildContext context) { | ||
VibrationController.instance.vibrate(VibrationType.light); | ||
Navigator.of(context) | ||
.push(MaterialPageRoute(builder: (context) => HomePage())); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed as you already called vibration on ready_for_session_page.dart
which call this method
() { | ||
VibrationController.instance.vibrate(VibrationType.light); | ||
callback(); | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nicer to extract that to a method
Hey ! I just added new commit let me know this is what u want ? |
void onDone(BuildContext context) { | ||
Navigator.of(context) | ||
.push(MaterialPageRoute(builder: (context) => HomePage())); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert to arrow function
void handleCallback() => _handleCallback(); | ||
|
||
void _handleCallback() { | ||
VibrationController.instance.vibrate(VibrationType.light); | ||
callback(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no reason to call function to call function??.
Also move it above the build
method
scaffold: false, | ||
title: 'start_session', | ||
child: ReadyForSessionOrganism( | ||
callback, | ||
() => handleCallback(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can write this code cleaner by changing () => handleCallback(),
to just handleCallback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also this pr have conflicts with dev branch, please fix it as well
I have started to work on a big feature, it will take a couple of days and I want this feature to be in the same release. |
The big future got merged so I will continue from here as this pr didn't got new commits from last week. |
pr #117
Hey! I just add the missing vibration on intro page and ready_for_session_page .
let me know is this correct or not?