Fix #3894 : Added bottomsheet #4099

bkaur-bkj · 2022-01-14T05:39:18Z

Explanation

Essential Checklist

The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
Any changes to scripts/assets files have their rationale included in the PR explanation.
The PR follows the style guide.
The PR does not contain any unnecessary code changes from Android Studio (reference).
The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

If your PR includes UI-related changes, then:

Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes
For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see RTL guide)
Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

bkaur-bkj · 2022-01-14T05:43:46Z

@rt4914 I created the bottom sheet fragment and tried implementing listeners on help and options button but I can't figure out a way to call the original function from within the fragment. I have also tried to paste the block of code in the fragment itself but it requires some dagger-related classes. PTAL

rt4914 · 2022-01-19T17:53:07Z

@rt4914 I created the bottom sheet fragment and tried implementing listeners on help and options button but I can't figure out a way to call the original function from within the fragment. I have also tried to paste the block of code in the fragment itself but it requires some dagger-related classes. PTAL

Have a look at

oppia-android/app/src/main/java/org/oppia/android/app/player/stopplaying/StopExplorationDialogFragment.kt

Line 35 in 3a0afb4

val stopStatePlayingSessionListener: StopStatePlayingSessionListener =

where we attach an interface to activity and similar to this link call the function of that interface. You can do something similar to communicate from BottomSheet to activity/fragment.

@bkaur-bkj I have tried the accessibility part and I have shared this video which shows how we don't require the "Close" item.

device-2022-01-19-232038.mp4

bkaur-bkj · 2022-01-20T13:43:05Z

sir, I tried accessibility with bottom sheet after bottomsheet gets open if I swipe up my bottomsheet gets closed directly I don't get any such actions option. I see its fine but don't know why it shows different way

rt4914 · 2022-01-21T06:45:50Z

sir, I tried accessibility with bottom sheet after bottomsheet gets open if I swipe up my bottomsheet gets closed directly I don't get any such actions option. I see its fine but don't know why it shows different way

@bkaur-bkj Can you please record and share the video?

bkaur-bkj · 2022-01-21T06:55:37Z

@rt4914 I am facing few dependency errors which I am unable to resolve -
symbol: class BottomSheetItemClickListener location: package org.oppia.android.app.player.explorationC:\Users\Asus\Desktop\git_practice\oppia-android\app\build\tmp\kapt3\stubs\debug\org\oppia\android\app\player\exploration\ExplorationActivity.java:9: error: [ComponentProcessor:MiscError] dagger.internal.codegen.ComponentProcessor was unable to process this class because not all of its dependencies could be resolved. Check for compilation errors or a circular dependency with generated code.

bkaur-bkj · 2022-01-21T06:56:41Z

Once this issue is resolved, I will mark the PR ready for review.

bkaur-bkj · 2022-01-22T11:03:11Z

sir, I tried accessibility with bottom sheet after bottomsheet gets open if I swipe up my bottomsheet gets closed directly I don't get any such actions option. I see its fine but don't know why it shows different way

@bkaur-bkj Can you please record and share the video?

https://drive.google.com/file/d/16pGyh9XBpd7eCYU7iz6IvuQ1YIZkPzIN/view?usp=sharing this is the link of video

rt4914

@bkaur-bkj Thanks for the video. Its a bit unexpected but anyways we will be add close option to sheet.

app/src/main/java/org/oppia/android/app/fragment/BottomSheetFragment.kt

rt4914 · 2022-01-28T16:33:55Z

@bkaur-bkj I just checked out into your branch and ran the app and its working without any error. Can you try re-building once again? If the error still continues then we can meet and discuss it.

bkaur-bkj · 2022-01-29T12:50:04Z

@bkaur-bkj I just checked out into your branch and ran the app and its working without any error. Can you try re-building once again? If the error still continues then we can meet and discuss it.

yes I tried rebuild, thanks it worked

bkaur-bkj · 2022-01-29T12:51:33Z

@rt4914 the functionality is working fine now, let me know the changes and should I add close option then? as u refused before but you mentioned in the last review that we will be adding it

rt4914 · 2022-03-01T17:20:49Z

testExploration_openBottomsheet_selectOptionsInBottomsheet_opensOptionsActivity

@bkaur-bkj Is your code updated to get this error? I checked out in your code and ran this test locally and it passed - twice.

For this I used Pixel 3A API 28

bkaur-bkj · 2022-03-07T06:11:47Z

testExploration_openBottomsheet_selectOptionsInBottomsheet_opensOptionsActivity

@bkaur-bkj Is your code updated to get this error? I checked out in your code and ran this test locally and it passed - twice.

For this I used Pixel 3A API 28

But why are the same checks failing on Github Actions. PTAL

rt4914 · 2022-03-07T19:03:20Z

@bkaur-bkj For now I have re-ran the CI checks as we have noticed something similar on #4183 also.

oppiabot · 2022-03-14T19:54:55Z

Hi @bkaur-bkj, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

bkaur-bkj · 2022-03-16T11:58:01Z

@bkaur-bkj For now I have re-ran the CI checks as we have noticed something similar on #4183 also.

Okay sir but I am not clear then how I should proceed as the checks failed again.

BenHenning

Thanks @bkaur-bkj. Took a pass--PTAL.

I'm also going to dig into why the CI checks aren't running; I'm going to see if I can get them to restart.

BenHenning · 2022-03-24T06:12:47Z

app/src/main/java/org/oppia/android/app/player/exploration/BottomSheetOptionsMenu.kt

+import org.oppia.android.databinding.FragmentBottomSheetBinding
+
+/** Bottom sheet fragment for displaying options menu */
+class BottomSheetOptionsMenu(val internalProfileId: Int) : BottomSheetDialogFragment() {


Does this parameter need to be passed in through arguments, instead, since Android can recreate the fragment itself?

BenHenning · 2022-03-24T06:13:01Z

app/src/main/java/org/oppia/android/app/player/exploration/BottomSheetOptionsMenu.kt

+  private fun setUpOnClickListeners(binding: FragmentBottomSheetBinding) {
+    val bottomSheetItemClickListener = activity as BottomSheetOptionsMenuItemClickListener
+    binding.actionHelp.setOnClickListener {
+


Nit: please remove extra newline for consistency. Ditto below.

BenHenning · 2022-03-24T06:14:41Z

...ain/java/org/oppia/android/app/player/exploration/BottomSheetOptionsMenuItemClickListener.kt

+   * Function for passing the selected itemId
+   *
+   * @param itemId: Resource Id of the option selected
+   * @return [Unit]


This isn't needed for Unit return types.

BenHenning · 2022-03-24T06:14:49Z

...ain/java/org/oppia/android/app/player/exploration/BottomSheetOptionsMenuItemClickListener.kt

+  /**
+   * Function for passing the selected itemId
+   *
+   * @param itemId: Resource Id of the option selected


Suggested change

* @param itemId: Resource Id of the option selected

* @param itemId resource Id of the option selected

BenHenning · 2022-03-24T06:15:21Z

...ain/java/org/oppia/android/app/player/exploration/BottomSheetOptionsMenuItemClickListener.kt

+   * @param itemId: Resource Id of the option selected
+   * @return [Unit]
+   */
+  fun handleOnOptionsItemSelected(itemId: Int)


Perhaps use an enum instead of a resource ID to limit the possibilities of what can be passed to this function (limiting possibilities generally results in more error-tolerant and understandable code).

BenHenning · 2022-03-24T06:19:14Z

app/src/main/res/layout/fragment_bottom_sheet.xml

@@ -0,0 +1,44 @@
+<?xml version="1.0" encoding="utf-8"?>


The name of this file should match its location since all layout resources are global to the app.

BenHenning · 2022-03-24T06:19:46Z

app/src/main/res/layout/exploration_activity.xml

@@ -55,6 +55,14 @@
            android:scaleType="center"
            app:srcCompat="@{viewModel.isAudioStreamingOn ? @drawable/ic_audio_streaming_on_24dp : @drawable/ic_audio_streaming_off_24dp}"
            android:visibility="@{viewModel.showAudioButton ? View.VISIBLE : View.GONE}" />
+
+          <ImageButton
+            android:id="@+id/action_bottom_sheet"


IDs are global to the app--this should be more specific to what specific menu it corresponds to.

Ditto for the options in the bottom sheet fragment layout below.

BenHenning · 2022-03-24T06:20:50Z

app/src/main/res/layout/fragment_bottom_sheet.xml

+  android:id="@+id/bottom_sheet_layout"
+  android:layout_width="match_parent"
+  android:layout_height="wrap_content"
+  android:padding="10dp"
+  android:orientation="vertical">


Indentation seems off here--please fix.

BenHenning · 2022-03-24T06:22:13Z

app/src/sharedTest/java/org/oppia/android/app/player/exploration/ExplorationActivityTest.kt

@@ -444,7 +443,7 @@ class ExplorationActivityTest {
  }

  @Test
-  fun testExploration_overflowMenu_isDisplayedSuccessfully() {
+  fun testExploration_openBottomSheet() {


What's the expected outcome? The action seems to be opening the bottom sheet, but it's not clear what this test is making sure works.

Perhaps instead:

testActivity_initialState_openBottomSheet_showsBottomSheet

Might help clarify?

BenHenning · 2022-03-24T06:23:17Z

app/src/sharedTest/java/org/oppia/android/app/player/exploration/ExplorationActivityTest.kt

@@ -1839,7 +1867,8 @@ class ExplorationActivityTest {

  private fun addShadowMediaPlayerException(dataSource: Any, exception: Exception) {
    val classLoader = ExplorationActivityTest::class.java.classLoader!!
-    val shadowMediaPlayerClass = classLoader.loadClass("org.robolectric.shadows.ShadowMediaPlayer")
+    val shadowMediaPlayerClass =
+      classLoader.loadClass("org.robolectric.shadows.ShadowMediaPlayer")


Why this change? Ditto above. I suspect these might be extra and unnecessary reformats done by Android Studio.

BenHenning · 2022-03-24T06:30:28Z

It looks like CI checks are running now. It may help also to merge the latest develop in @bkaur-bkj as it contains improvements for Gradle runs.

oppiabot · 2022-03-31T07:46:52Z

Hi @bkaur-bkj, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

oppiabot · 2022-04-07T08:24:07Z

Hi @bkaur-bkj, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

draft implementation

53bfa03

bkaur-bkj requested a review from rt4914 January 14, 2022 05:39

bkaur-bkj assigned rt4914 Jan 14, 2022

rt4914 assigned bkaur-bkj and unassigned rt4914 Jan 19, 2022

interface implemented

c679afa

bkaur-bkj assigned rt4914 and unassigned bkaur-bkj Jan 21, 2022

rt4914 suggested changes Jan 25, 2022

View reviewed changes

app/src/main/java/org/oppia/android/app/fragment/BottomSheetFragment.kt Outdated Show resolved Hide resolved

rt4914 assigned bkaur-bkj and unassigned rt4914 Jan 25, 2022

bkaur-bkj assigned rt4914 and unassigned bkaur-bkj Jan 26, 2022

rt4914 assigned bkaur-bkj and unassigned rt4914 Jan 28, 2022

bkaur-bkj marked this pull request as ready for review January 29, 2022 12:50

bkaur-bkj requested a review from BenHenning as a code owner January 29, 2022 12:50

bkaur-bkj added 3 commits January 29, 2022 18:23

functionality added

f7d9af6

newlines added

657c292

lint

8658454

rt4914 assigned bkaur-bkj and unassigned rt4914 Mar 1, 2022

bkaur-bkj assigned rt4914 and unassigned bkaur-bkj Mar 7, 2022

rt4914 assigned bkaur-bkj and unassigned rt4914 Mar 7, 2022

oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Mar 14, 2022

oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Mar 16, 2022

Merge branch 'develop' into overflow-to-bottomsheet

68808cb

bkaur-bkj assigned rt4914 and unassigned bkaur-bkj Mar 16, 2022

rt4914 assigned BenHenning Mar 22, 2022

BenHenning reviewed Mar 24, 2022

View reviewed changes

BenHenning closed this Mar 24, 2022

BenHenning reopened this Mar 24, 2022

BenHenning assigned bkaur-bkj and unassigned BenHenning Mar 24, 2022

oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Mar 31, 2022

Merge branch 'oppia:develop' into overflow-to-bottomsheet

e038e34

oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Mar 31, 2022

oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Apr 7, 2022

oppiabot bot closed this Apr 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #3894 : Added bottomsheet #4099

Fix #3894 : Added bottomsheet #4099

bkaur-bkj commented Jan 14, 2022

bkaur-bkj commented Jan 14, 2022

rt4914 commented Jan 19, 2022

bkaur-bkj commented Jan 20, 2022

rt4914 commented Jan 21, 2022

bkaur-bkj commented Jan 21, 2022

bkaur-bkj commented Jan 21, 2022 •

edited

Loading

bkaur-bkj commented Jan 22, 2022

rt4914 left a comment

rt4914 commented Jan 28, 2022

bkaur-bkj commented Jan 29, 2022

bkaur-bkj commented Jan 29, 2022

rt4914 commented Mar 1, 2022

bkaur-bkj commented Mar 7, 2022

rt4914 commented Mar 7, 2022 •

edited

Loading

oppiabot bot commented Mar 14, 2022

bkaur-bkj commented Mar 16, 2022 •

edited

Loading

BenHenning left a comment

BenHenning Mar 24, 2022

BenHenning Mar 24, 2022

BenHenning Mar 24, 2022

BenHenning Mar 24, 2022

BenHenning Mar 24, 2022

BenHenning Mar 24, 2022

BenHenning Mar 24, 2022

BenHenning Mar 24, 2022

BenHenning Mar 24, 2022

BenHenning Mar 24, 2022

BenHenning commented Mar 24, 2022 •

edited

Loading

oppiabot bot commented Mar 31, 2022

oppiabot bot commented Apr 7, 2022

	* @param itemId: Resource Id of the option selected
	* @param itemId resource Id of the option selected

Fix #3894 : Added bottomsheet #4099

Fix #3894 : Added bottomsheet #4099

Conversation

bkaur-bkj commented Jan 14, 2022

Explanation

Essential Checklist

For UI-specific PRs only

bkaur-bkj commented Jan 14, 2022

rt4914 commented Jan 19, 2022

bkaur-bkj commented Jan 20, 2022

rt4914 commented Jan 21, 2022

bkaur-bkj commented Jan 21, 2022

bkaur-bkj commented Jan 21, 2022 • edited Loading

bkaur-bkj commented Jan 22, 2022

rt4914 left a comment

Choose a reason for hiding this comment

rt4914 commented Jan 28, 2022

bkaur-bkj commented Jan 29, 2022

bkaur-bkj commented Jan 29, 2022

rt4914 commented Mar 1, 2022

bkaur-bkj commented Mar 7, 2022

rt4914 commented Mar 7, 2022 • edited Loading

oppiabot bot commented Mar 14, 2022

bkaur-bkj commented Mar 16, 2022 • edited Loading

BenHenning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenHenning commented Mar 24, 2022 • edited Loading

oppiabot bot commented Mar 31, 2022

oppiabot bot commented Apr 7, 2022

bkaur-bkj commented Jan 21, 2022 •

edited

Loading

rt4914 commented Mar 7, 2022 •

edited

Loading

bkaur-bkj commented Mar 16, 2022 •

edited

Loading

BenHenning commented Mar 24, 2022 •

edited

Loading