Fixes #2824: [A11y] Add label for RecentlyPlayedActivity #3065
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,16 @@ | ||||||||||
| package org.oppia.android.app.home.recentlyplayed | ||||||||||
|
|
||||||||||
| import android.content.Context | ||||||||||
| import androidx.annotation.StringRes | ||||||||||
| import org.oppia.android.R | ||||||||||
|
|
||||||||||
| /** Represents different titles for RecentlyPlayedActivity used by screen readers. */ | ||||||||||
| enum class RecentlyPlayedTitleEnum(@StringRes private var title: Int) { | ||||||||||
| RECENTLY_PLAYED_STORIES(R.string.recently_played_stories), | ||||||||||
|
There was a problem hiding this comment. Why isn't this using recently_played_activity_title? There was a problem hiding this comment. Correct. Used |
||||||||||
| STORIES_FOR_YOU(R.string.stories_for_you); | ||||||||||
|
|
||||||||||
| /** Returns the string corresponding to this error's string resources, or null if there is none. */ | ||||||||||
| fun getTitleFromStringRes(context: Context): String? { | ||||||||||
| return title.let(context::getString) | ||||||||||
| } | ||||||||||
|
There was a problem hiding this comment.
Suggested change
The scope function way seems less readable to me, but YMMV. I also changed this to an optional single-assignment syntax. There was a problem hiding this comment. Updated as per suggestion. |
||||||||||
| } | ||||||||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,55 @@ | ||
| <?xml version="1.0" encoding="utf-8"?> | ||
| <layout xmlns:android="http://schemas.android.com/apk/res/android" | ||
| xmlns:app="http://schemas.android.com/apk/res-auto"> | ||
|
|
||
| <androidx.constraintlayout.widget.ConstraintLayout | ||
| android:layout_width="match_parent" | ||
| android:layout_height="match_parent" | ||
| android:background="@color/white"> | ||
|
|
||
| <com.google.android.material.appbar.AppBarLayout | ||
| android:id="@+id/recently_played_app_bar_layout" | ||
| android:layout_width="0dp" | ||
| android:layout_height="wrap_content" | ||
| android:theme="@style/ThemeOverlay.AppCompat.Dark.ActionBar" | ||
| app:layout_constraintEnd_toEndOf="parent" | ||
| app:layout_constraintStart_toStartOf="parent" | ||
| app:layout_constraintTop_toTopOf="parent"> | ||
|
|
||
| <androidx.appcompat.widget.Toolbar | ||
| android:id="@+id/recently_played_toolbar" | ||
| android:layout_width="match_parent" | ||
| android:layout_height="?attr/actionBarSize" | ||
| android:background="?attr/colorPrimary" | ||
| android:fontFamily="sans-serif" | ||
| android:minHeight="?attr/actionBarSize" | ||
| app:navigationContentDescription="@string/navigate_up" | ||
| app:navigationIcon="?attr/homeAsUpIndicator" | ||
| app:title="@string/recently_played_activity_title" | ||
| app:titleTextAppearance="@style/ToolbarTextAppearance" | ||
| app:titleTextColor="@color/white" /> | ||
| </com.google.android.material.appbar.AppBarLayout> | ||
|
|
||
| <FrameLayout | ||
| android:layout_width="0dp" | ||
| android:layout_height="0dp" | ||
| app:layout_constraintBottom_toBottomOf="parent" | ||
| app:layout_constraintEnd_toEndOf="parent" | ||
| app:layout_constraintStart_toStartOf="parent" | ||
| app:layout_constraintTop_toBottomOf="@id/recently_played_app_bar_layout"> | ||
|
|
||
| <FrameLayout | ||
| android:id="@+id/recently_played_fragment_placeholder" | ||
| android:layout_width="match_parent" | ||
| android:layout_height="match_parent" /> | ||
|
|
||
| <View | ||
| android:layout_width="match_parent" | ||
| android:layout_height="6dp" | ||
| app:layout_constraintBottom_toBottomOf="parent" | ||
| app:layout_constraintEnd_toEndOf="parent" | ||
| app:layout_constraintStart_toStartOf="parent" | ||
| app:layout_constraintTop_toBottomOf="@id/recently_played_app_bar_layout" /> | ||
|
There was a problem hiding this comment. Is it value to have constraints when within a layout being constrained? Do we eve need these? There was a problem hiding this comment. These constraints are incorrect. Removed now. |
||
| </FrameLayout> | ||
| </androidx.constraintlayout.widget.ConstraintLayout> | ||
| </layout> | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 seems reasonable to me, but I think we want to do a push toward using protos more aggressively for sending arguments & intent extras. Maybe just go ahead and define an intent extra proto for this activity & use that, instead? It means a less-clean enum but you could still create an extension function for
getTitleFromStringResto keep dependent code clean.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 have tried to use protos. I am not sure if its correct so do let me know WDYT.
DestinationScreenwill helps to identify which activity to open and also with what intent extra values.RecentlyPlayedActivityIntentExtrasis a value insideDestinationScreenwhich contains information about the destination activity as well as its intent extras.activitypackage I have createdActivityRouterwhich will be injected to all activities / fragments from where we want to start the next activity. In this we will callrouteToScreenwith correctDestinationScreenand then this will decide which activity should be started.RecentlyPlayedActivityIntentExtrasproto anytime it will be very easy to make changes to code. For example in current develop as I had to send an enum I had to introduced a new key and change thecreateRecentlyPlayedActivityIntentfunction by introducing a new argument which in turn resulted in changes in all the instances where that function was getting called. But with this new approach these changes will be comparatively less.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.
Ah, I was just thinking that you could pass a RecentlyPlayedActivityIntentExtras proto here & proceed similarly. The router approach seems much nicer. Do you want to do that change as part of this PR or separately?