-
Notifications
You must be signed in to change notification settings - Fork 526
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 #2132: Chapter List Page Issues (#2232) - Blur and lock thumbnails of inactive cards using realtimeblurview #2293
Conversation
Mock |
@anandwana001 Please review. Not sure how to handle Bazel failure, did I not include something in this PR? |
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, @viktoriias
Did the first pass.
@@ -109,6 +109,7 @@ dependencies { | |||
'androidx.work:work-runtime-ktx:2.4.0', | |||
'com.chaos.view:pinview:1.4.3', | |||
'com.github.bumptech.glide:glide:4.11.0', | |||
'com.github.mmin18:realtimeblurview:1.2.1', |
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.
We need to check a few things before using the new library:
- is it working correctly with Bazel?
- Is there any license issue?
- Do we have any other core android approach where we can make it work without any library?
@@ -109,6 +109,7 @@ dependencies { | |||
'androidx.work:work-runtime-ktx:2.4.0', | |||
'com.chaos.view:pinview:1.4.3', | |||
'com.github.bumptech.glide:glide:4.11.0', | |||
'com.github.mmin18:realtimeblurview:1.2.1', |
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.
As for Bazel, we also need to add a dependency to app module-level BUILD.bazel file.
https://github.com/oppia/oppia-android/blob/develop/app/BUILD.bazel
android:width="80dp" | ||
android:height="80dp" | ||
android:autoMirrored="true" | ||
android:tint="#FFFFFFFF" |
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.
Is this a core android icon?
We can use dimens.xml , color.xml res file for values.
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 got it in vector assets. Other ic_
vector images don't use dimens.xml , color.xml. Is that a convention?
@@ -53,6 +53,38 @@ | |||
app:layout_constraintTop_toTopOf="parent" | |||
app:lessonThumbnail="@{viewModel.chapterThumbnail}" /> | |||
|
|||
<com.github.mmin18.widget.RealtimeBlurView |
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.
How about if we use Render script?
reference - https://futurestud.io/tutorials/glide-custom-transformation
@rt4914 Any thoughts on 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.
I looked into the RealtimeBlurView Library it uses RenderScript so it would be better rather using the library
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.
@anandwana001 I personally prefer non-third-party-library solution. I think we should create a separate PR using this script and see results and based on that finalise things.
To clarify on one point, @mschanteltc should we always be using the same colored overlay/lock for all chapter thumbnails? If so, what color + opacity should we be using for that overlay? Also, which material lock icon should we be using for the lock? |
Original requirements are here: #2132 (comment) |
Thanks for clarifying @viktoriias! @mschanteltc does the image linked above look correct per the spec? |
Thank you @viktoriias. Chapter Thumbnail backgrounds aren't supposed to be white, but I believe your example just used a sample image with a white background. Once the images are actually assigned to one of our preselected background colors, the blur would be set to the image's background color and work as intended. |
Hi @viktoriias, are you still working on this? Also, please write appropriate PR description. For reference, you can take a look at the instructions mentioned in point 4. https://github.com/oppia/oppia-android/wiki#instructions-for-making-a-code-change |
@Luffy18346 I will update or un-assign myself by the end of the week. |
Ok but please update PR Explanation before un-assigning yourself. |
Explanation
This PR fixes part of issue #2132. Here I blur and lock thumbnails of inactive cards.
Checklist