Fix #5039 : Align policy text and symbols to the left, partial fix for list items #5573
Conversation
|
Hey @adhiamboperes , I have created a draft PR addressing #5039 , which focuses on ensuring that policy text and symbols (such as bullet points and question marks) are aligned to the left in an LTR format. While this change improves the overall text alignment, there remains an outstanding issue with the alignment of Issue Summary:Although most text and individual symbols are now correctly aligned, list items created using the Request for Guidance:I kindly ask for your insights or recommendations on potential solutions for handling the alignment of
Thanks !! |
|
Hey @adhiamboperes PTAL |
|
Unassigning @TanishMoral11 since a re-review was requested. @TanishMoral11, please make sure you have addressed all review comments. Thanks! |
adhiamboperes
changed the title
|
Thanks @TanishMoral11! The solution for bullets look good. As for the items in val policyDescription and use regex to extract the reguired tag (in this case li) and apply the necessary formatting, then setting the mofidified string to the textview. E.g. in https://github.com/oppia/oppia-android/pull/5286/files and https://github.com/oppia/oppia-android/pull/5212/files. |
|
Hey @adhiamboperes , PTAL 2024-11-13_00-39-44.mp4 |
|
Unassigning @TanishMoral11 since a re-review was requested. @TanishMoral11, please make sure you have addressed all review comments. Thanks! |
TanishMoral11
changed the title
There was a problem hiding this comment.
Thanks @TanishMoral11.
While the solution appears correct, a number of cleanup changes are required to get the PR merge ready.
Also, the video you have uploaded in the comments does not play. Please re-upload it to the PR description and ensure it can play.
utility/src/main/java/org/oppia/android/util/parser/html/HtmlParser.kt
Outdated
Show resolved
Hide resolved
utility/src/main/java/org/oppia/android/util/parser/html/HtmlParser.kt
Outdated
Show resolved
Hide resolved
utility/src/main/java/org/oppia/android/util/parser/html/HtmlParser.kt
Outdated
Show resolved
Hide resolved
utility/src/main/java/org/oppia/android/util/parser/html/HtmlParser.kt
Outdated
Show resolved
Hide resolved
utility/src/main/java/org/oppia/android/util/parser/html/LiTagHandler.kt
Outdated
Show resolved
Hide resolved
utility/src/main/java/org/oppia/android/util/parser/html/ListItemLeadingMarginSpan.kt
Outdated
Show resolved
Hide resolved
|
Hey @adhiamboperes PTAL , Thank you for your detailed review and valuable feedback. I have addressed all your requested adjustments, including:
Additionally, I have re-uploaded the video demonstration to the PR description, ensuring it is now accessible and plays correctly. Thank you again for your guidance. I look forward to your review of these updates. Please let me know if there are any further adjustments needed. 2024-11-13_00-39-44.mp4 |
|
Unassigning @TanishMoral11 since a re-review was requested. @TanishMoral11, please make sure you have addressed all review comments. Thanks! |
|
Thanks @adhiamboperes , PTAL !!
I Added
Yeah, You're Right About The Package Location. I Moved
I've Tested The App, And As Yet No Misbehavior Was Detected Related To These Changes. |
|
Unassigning @TanishMoral11 since a re-review was requested. @TanishMoral11, please make sure you have addressed all review comments. Thanks! |
There was a problem hiding this comment.
Apologies for taking a while to review this, @TanishMoral11.
A few points to note on Bazel:
- I have taken a look at your CI failures. The latest error is
app/policies/PoliciesFragmentPresenter_updated.kt:78:56: warning: parameter 'lineIndex' is never used, could be renamed to _ parsedHtmlDescription.split("\n").forEachIndexed { lineIndex, line ->. Unless you fix this error, you won't be able to see if there are additional errors. - You can read this wiki page for information on how to read Bazel CI errors. A youtube video is also linked.
- You should set aside some time to set up the project build in Bazel, so that you can run Bazel builds locally. Additionally, some more advanced issues you'll encounter will require you to have Bazel set up. The discussions section will be helpful in searching for solutions to some common setup issues.
APK & AAB differences analysisNote that this is a summarized snapshot. See the CI artifacts for detailed differences. DevExpand to see flavor specificsUniversal APKAPK file size: 19 MiB (old), 19 MiB (new), 1548 bytes (Added) APK download size (estimated): 17 MiB (old), 17 MiB (new), 1585 bytes (Added) Method count: 260202 (old), 260198 (new), 4 (Removed) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6818 (old), 6818 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 18 MiB (old), 18 MiB (new), 1552 bytes (Added) Configuration hdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 49 KiB (old), 49 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 45 KiB (old), 45 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 86 KiB (old), 86 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 57 KiB (old), 57 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 0 bytes (No change) AlphaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 264 bytes (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 36 bytes (Added) Method count: 116280 (old), 116271 (new), 9 (Removed) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5786 (old), 5785 (new), 1 (Removed)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 264 bytes (Removed)
Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) BetaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 0 bytes (No change) APK download size (estimated): 10 MiB (old), 10 MiB (new), 1113 bytes (Added) Method count: 116286 (old), 116277 (new), 9 (Removed) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5786 (old), 5785 (new), 1 (Removed)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 0 bytes (No change)
Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) GaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 52 bytes (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 1112 bytes (Removed) Method count: 116286 (old), 116277 (new), 9 (Removed) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5786 (old), 5785 (new), 1 (Removed)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 48 bytes (Removed)
Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) |
|
Hey @adhiamboperes , PTAL !! |
|
Unassigning @TanishMoral11 since a re-review was requested. @TanishMoral11, please make sure you have addressed all review comments. Thanks! |
|
Hi @TanishMoral11, 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. |
oppiabot
bot
added
the
stale
TanishMoral11
removed
the
stale
|
Hi @TanishMoral11, 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. |
oppiabot
bot
added
the
stale
TanishMoral11
removed
the
stale
There was a problem hiding this comment.
Thanks @TanishMoral11! I only have comments for one file, PTAL.
I have updated the branch from develop too re-trigger CI so we can see the CI failures, the logs had already expired.
|
|
||
| private val bulletDiameter by lazy { bulletRadius * 2 } | ||
| private val baseMargin = context.resources.getDimensionPixelSize((spacing_before_bullet)) | ||
|
|
||
| private val isRtl by lazy { |
There was a problem hiding this comment.
This variable appears unused.
| @@ -123,17 +119,12 @@ sealed class ListItemLeadingMarginSpan : LeadingMarginSpan { | |||
| private val displayLocale: OppiaLocale.DisplayLocale | |||
| ) : ListItemLeadingMarginSpan() { | |||
| private val resources = context.resources | |||
There was a problem hiding this comment.
This and displayLocale above are nolonger used.
| private val isRtl by lazy { | ||
| displayLocale.getLayoutDirection() == ViewCompat.LAYOUT_DIRECTION_RTL | ||
| } | ||
| 2 * longestNumberedItemPrefix.length + baseMargin |
There was a problem hiding this comment.
Even though you have refactored the computation of this, I don't see any current usages.
APK & AAB differences analysisNote that this is a summarized snapshot. See the CI artifacts for detailed differences. DevExpand to see flavor specificsUniversal APKAPK file size: 19 MiB (old), 19 MiB (new), 1168 bytes (Added) APK download size (estimated): 17 MiB (old), 17 MiB (new), 2374 bytes (Added) Method count: 260202 (old), 260198 (new), 4 (Removed) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6818 (old), 6818 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 18 MiB (old), 18 MiB (new), 1168 bytes (Added) Configuration hdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 49 KiB (old), 49 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 45 KiB (old), 45 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 86 KiB (old), 86 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 57 KiB (old), 57 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 0 bytes (No change) AlphaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 120 bytes (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 566 bytes (Added) Method count: 116281 (old), 116272 (new), 9 (Removed) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5786 (old), 5785 (new), 1 (Removed)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 116 bytes (Removed)
Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) BetaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 32 bytes (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 771 bytes (Added) Method count: 116287 (old), 116278 (new), 9 (Removed) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5786 (old), 5785 (new), 1 (Removed)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 32 bytes (Removed)
Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) GaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 4 bytes (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 600 bytes (Removed) Method count: 116287 (old), 116278 (new), 9 (Removed) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5786 (old), 5785 (new), 1 (Removed)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 4 bytes (Added)
Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) |
Fix #5039: This PR addresses the issue described by ensuring that policy text and special symbols (such as bullet points and question marks) within policy descriptions are aligned to the left. The content now displays correctly in an LTR (left-to-right) format. However, this is only a partial fix as there are still outstanding issues with the alignment of
<li>(list item) tags.Implemented Changes:
LeftAlignedSymbolsSpanto ensure text and symbols are left-aligned.PoliciesFragmentPresenterto parse and format the text appropriately for LTR display.Known Limitations:
<li>tags within parsed HTML content is not fully resolved. Further work is required to properly align list items.Essential Checklist
develop.Progress Till Now :-
2024-11-07_19-52-22.mp4