-
Notifications
You must be signed in to change notification settings - Fork 806
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
Added a pause button #109
base: master
Are you sure you want to change the base?
Added a pause button #109
Conversation
Included a pauses button as a fab.
Added a fab for the pause button
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.
Hi! I reviewed your PR.
Did you test your changes using device or emulator?
app/src/main/java/com/danielkim/soundrecorder/fragments/RecordFragment.java
Outdated
Show resolved
Hide resolved
mPauseButton.setImageResource(R.drawable.ic_media_pause); | ||
mPauseButton.setColorNormal(Color.parseColor("#F44336")); | ||
mPauseButton.setColorRipple(Color.parseColor("#C0C0C0")); | ||
mPauseButton.setColorPressed(Color.parseColor("#727272")); |
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.
Why do you set app:fab_colorPressed="@color/secondary_text"
in the layout and then override it here in the code? If you do it in the layout file only, the code will be cleaner.
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.
Indeed!
mPauseButton.setCompoundDrawablesWithIntrinsicBounds | ||
(R.drawable.ic_media_pause ,0 ,0 ,0); | ||
mPauseButton.setImageResource(R.drawable.ic_media_pause); | ||
mPauseButton.setColorNormal(Color.parseColor("#F44336")); |
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.
Why do you set fab:fab_colorNormal="@color/primary"
in the layout and then override it here in the code?
android:gravity="center_horizontal" | ||
android:orientation="horizontal"> | ||
|
||
<com.melnykov.fab.FloatingActionButton |
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.
Do you know that com.melnykov.fab.FloatingActionButton
view is DEPRECATED? This is stated in makovkastar/FloatingActionButton repository readme. The author also suggests to use the FloatingActionButton from the support library instead.
android:layout_centerHorizontal="true" | ||
android:src="@drawable/ic_mic_white_36dp" /> | ||
|
||
<com.melnykov.fab.FloatingActionButton |
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.
The same for pause button. com.melnykov.fab.FloatingActionButton
view is DEPRECATED.
@@ -1,20 +1,46 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android" | |||
xmlns:app="http://schemas.android.com/apk/res-auto" | |||
xmlns:fab="http://schemas.android.com/apk/res-auto" |
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.
xmlns:fab
is the same as xmlns:app
. Why do you need to redefine this namespace?
@@ -167,18 +170,24 @@ public void onChronometerTick(Chronometer chronometer) { | |||
private void onPauseRecord(boolean pause) { |
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.
Remove TODO, if you've implemented this.
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.
Thanks for reviewing my PR. I will make the changes. :). I used a device to test.
No description provided.