Added a pause button #109
Added a pause button #109
Conversation
Included a pauses button as a fab.
Added a fab for the pause button
naXa777
left a comment
naXa777
left a comment
There was a problem hiding this comment.
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.
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.
| 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.
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.
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.
The same for pause button. com.melnykov.fab.FloatingActionButton view is DEPRECATED.
| <?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.
xmlns:fab is the same as xmlns:app. Why do you need to redefine this namespace?
| } | ||
|
|
||
| //TODO: implement pause recording | ||
| private void onPauseRecord(boolean pause) { |
There was a problem hiding this comment.
Remove TODO, if you've implemented this.
There was a problem hiding this comment.
Thanks for reviewing my PR. I will make the changes. :). I used a device to test.
3 participants
No description provided.