-
-
Notifications
You must be signed in to change notification settings - Fork 89
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
fix(app): Move random mode to navigation menu. #559
Conversation
I should've added a UI Test. I'll do it in a moment. |
Codecov Report
@@ Coverage Diff @@
## master #559 +/- ##
==========================================
+ Coverage 47.78% 47.90% +0.12%
==========================================
Files 33 34 +1
Lines 1871 1866 -5
Branches 180 179 -1
==========================================
Hits 894 894
+ Misses 930 925 -5
Partials 47 47
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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 if I wasn't clear earlier, but the intention was to remove the dialog fragment as well and make the random preset fragment like the rest of the screens, e.g. sleep timer.
Wouldn't require changing layouts. Would require to add a new fragment and move existing tests from SoundLibraryFragment.
Oh that makes more sense, I'll change and adapt to that solution which I think it's much better. |
Hey @ashutoshgngwr I started doing it now again, and I have some doubts as it seems pretty easy it's also a bit confusing. I understand that you want a fragment for random presets that is accesed by navigation menu. Once user click there, what is he expected to see on that fragment screen? PS: I already created a Fragment and user can acceess there but moving test from SoundLibraryFragment to the new RandomPresetFragment test requires more changes to make them work. Thank you! |
Essentially the same layout that was shown in the random preset dialog. When a user clicks the random preset menu item, they are taken to a fragment with the following form. PS, We can make the play button a floating action button. We can also move and expand the highlighted text to the top like other screens, e.g. Generated random presets will contain sounds corresponding to the chosen mood. Moreover, dense presets will attempt to play more sounds simultaneously.
Okay, first let's get everything else right. We will worry about the tests later. Let me know if I can help you with anything else. |
9c81312
to
87cbeb9
Compare
Tell me if you like the changes and what do you want we continue working on! Let me know how you're looking at them! |
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.
I also didn't see any deletions in SoundLibraryFragment. I am guessing you left it out for last.
<Button | ||
android:id="@+id/cancel_preset_button" | ||
style="?attr/buttonStyle" | ||
android:layout_width="wrap_content" | ||
android:layout_height="wrap_content" | ||
android:layout_marginTop="16dp" | ||
android:visibility="visible" | ||
android:text="@string/cancel" | ||
app:layout_constraintEnd_toStartOf="@id/play_preset_button" | ||
app:layout_constraintTop_toTopOf="parent" | ||
app:layout_constraintBottom_toBottomOf="parent" /> |
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.
This isn't really needed...? There's nothing to cancel.
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're right! I just followed the exact same picture layout, but it was still kinda weird for me looking at the buttons there, it looked pretty ugly, and as you said play button could be a fab! that's why I decided to push these changes before continue doing more work.
<Button | ||
android:id="@+id/play_preset_button" | ||
style="?attr/buttonStyle" | ||
android:layout_width="wrap_content" | ||
android:layout_height="wrap_content" | ||
android:layout_marginTop="16dp" | ||
android:visibility="visible" | ||
android:text="@string/save" | ||
app:layout_constraintEnd_toEndOf="parent" | ||
app:layout_constraintTop_toTopOf="parent" | ||
app:layout_constraintBottom_toBottomOf="parent"/> |
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.
It can be a floating action button.
} | ||
|
||
binding.cancelPresetButton.setOnClickListener { | ||
//What do we do cancel? Should we reset preferences 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.
It's not required in this case.
} | ||
MediaPlayerService.playRandomPreset(requireContext(), tag, intensity) | ||
//We'll add later a if player is playing then show else there was an error | ||
Toast.makeText(requireContext(), "Playing Random!", Toast.LENGTH_SHORT).show() |
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.
I think clicking the play button should just play a random preset regardless if the player was already playing. The logic remains the same. MediaPlayerService internally stops the existing playback before starting a random (or any) preset. There's no error scenario here per-say.
|
||
// Your choice to show up in-app review on this fragment! | ||
// maybe show in-app review dialog to the user | ||
//InAppReviewFlowManager.maybeAskForReview(requireActivity()) |
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.
It doesn't always result in the dialog being displayed. Sometimes it will be displayed or if the user has already completed the flow once, it won't be shown so it's a safe call.
<TextView | ||
android:layout_width="match_parent" | ||
android:layout_height="wrap_content" | ||
android:layout_marginVertical="7dp" | ||
android:text="@string/intensity_description" /> |
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.
Move this to the top of the layout and change the message. The message could be: Generated random presets will contain sounds corresponding to the chosen mood. Moreover, dense presets will attempt to play more sounds simultaneously.
android:layout_height="wrap_content" | ||
android:layout_marginBottom="7dp" | ||
android:text="@string/mood" | ||
android:textAppearance="?attr/textAppearanceHeadline5" |
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.
Opinion: This looks ugly in its own fragment. Maybe try headline 6 for both headings.
Hey @ashutoshgngwr, do you like the actual layout? Is the same as before but with implemented changes that you gave me. Let me know if there's anything I could improve on! |
@robercoding Looks good. Can be improved I guess. Other fragments have a padding of 20dp on their root layouts (inside scroll view). I think that would look good. The FAB doesn't look properly positioned on large screens. I am guessing you didn't wrap everything in Relative or Constraint layout, e.g. Relative/Constraint Layout -> FAB + (ScrollView -> Actual Fragment). The bottom-end corner is the conventional position for FABs. PS, another thing we could try is to orient the radio buttons horizontally. It is easily achievable using FlexboxLayout and we already have it as a dependency in the timer picker view. The final layout structure would look like RadioGroup -> FlexboxLayout -> RadioButton. Not sure if the results would be good, but definitely worth a shot. |
Hey @ashutoshgngwr, I've done these new changes and looks much better now!! Using Flexbox scenario with that structure layout (RadioGroup -> Flexbox -> RadioButtons) is impossible because RadioButtons are not controlled by RadioGroup and user can select all 3 options at the same time. But there's an easy way to achieve horizontal orientation by just setting radiogroup orientation as horizontal! I second that idea! ✌️ I'll push today as soon as I finish other parts! |
Well, using horizontal orientation means that any overflow will be clipped off-screen, so that doesn't work either. 😓 On a side note, increase padding between description text and mood heading. It looks a little cluttered right now. |
What do you think about using this library for horizontal view? It's built on top of FlexboyLayout! It doesn't overflow if you set more RadioButtons in the future! I couldn't find a way to center its radiobuttons (At least the options I've tried) but setting a minWidth and wrap content in width looks good on both. It's up to you to use this external library! Let me know if you like it. |
@robercoding It doesn't look mature, but if the library works, I guess it's fine. If something breaks in the future, we will figure something out. 😅 Everything looks great now! 👍 |
…et_fragment.xml. Deleted SoundLibraryFragment.kt random preset button and dialog_fragment__random_preset.xml. Changed intensity_description in strings.xml (Only english).
…last commit). Set manually radioButtons since is not working in XML. Remove 2 tests from SoundLibraryFragmentTest.kt and add 1 test to RandomPresetFragmentTest.kt
I've spent time wondering how it works to make play sounds and how it arrives to the UI and to the back until I just remembered you provided us a diagram on CONTRIBUTING.md. My bad there lol. |
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.
Needs a few styling changes, looks great otherwise. Thanks! 👍
Changes
Moved random mode to navigation menu!
Others