Fix #3822: Remove maxLines contsraint's to enable long text to linebreak #4547

KevinGitonga · 2022-09-01T11:41:59Z

Explanation

Fix #3822: This PR fixes Portuguese text cut off by completely removing maxLines contraint's used in the several variations of the onboarding_slide layouts on the title text . This allows for the text to easily break to the next line incase the text is very long to fit in a single line.

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

Phone screenshots

Before Portrait	After Portrait

Before Landscape	After Landscape

Tab screenshots

Before Portrait	After Portrait

Before Landscape	After Landscape

BenHenning · 2022-09-01T22:28:30Z

@KevinGitonga my only concern with the solution is it may not work for longer translations that we might encounter in the future. I wonder: is it reasonable to introduce line-wrapping here, instead, for cases when the title is too long?

@rt4914 curious about your thoughts here, too.

KevinGitonga · 2022-09-02T11:39:42Z

@BenHenning it actually makes sense to move the extra text to next line, I have updated maxLines from 1 to 2 to allow the extra text to move to the next line. This is with assumption that the translated text can never be more than 2 lines. I have reverted changes on end and start margins as this will not work and the device's screen gets thinner line wrapping would work better.

BenHenning · 2022-09-05T10:02:02Z

@BenHenning it actually makes sense to move the extra text to next line, I have updated maxLines from 1 to 2 to allow the extra text to move to the next line. This is with assumption that the translated text can never be more than 2 lines. I have reverted changes on end and start margins as this will not work and the device's screen gets thinner line wrapping would work better.

Thanks @KevinGitonga. Can you also update the screenshots in the PR? It's hard to verify the code just by looking at it, so the screenshots are really important to also convey the difference from a user's perspective.

I'm also assigning @rt4914 so that he can leave his thoughts.

rt4914

@KevinGitonga Please update your PR title and description as you are not making any change to margins.

Thanks.

rt4914 · 2022-09-07T12:04:00Z

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

@@ -45,7 +45,7 @@
        android:layout_marginBottom="16dp"
        android:fontFamily="sans-serif-medium"
        android:gravity="center_horizontal"
-        android:maxLines="1"
+        android:maxLines="2"


I think we should remove this maxLines attributes completely for this screen. So that even if in some screen it goes to three-lines the UI will be visible fully.

Changes Applied.

KevinGitonga · 2022-09-08T08:54:19Z

@KevinGitonga Please update your PR title and description as you are not making any change to margins.

Thanks.

Hey @rt4914 have made the changes as requested PTAL.

rt4914

@KevinGitonga PTAL

rt4914 · 2022-09-12T19:02:36Z

app/src/main/res/layout-sw600dp-port/onboarding_slide.xml

@@ -41,7 +41,7 @@
        android:layout_marginTop="48dp"
        android:layout_marginEnd="160dp"
        android:gravity="center_horizontal"
-        android:maxLines="1"
+        android:maxLines="2"


Same previous comment applies to this code too.

Remove maxLines attribute.

Also, please re-generate screenshots and update PR description just to be sure

rt4914 · 2022-09-12T19:02:41Z

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

@@ -43,7 +43,7 @@
        android:layout_marginBottom="16dp"
        android:fontFamily="sans-serif-medium"
        android:gravity="center_horizontal"
-        android:maxLines="1"
+        android:maxLines="2"


Same previous comment applies to this code too.

Remove maxLines attribute.

Also, please re-generate screenshots and update PR description just to be sure

rt4914 · 2022-09-12T19:02:44Z

app/src/main/res/layout-sw600dp-port/onboarding_slide_final.xml

@@ -43,7 +43,7 @@
        android:layout_marginEnd="160dp"
        android:fontFamily="sans-serif-medium"
        android:gravity="center_horizontal"
-        android:maxLines="1"
+        android:maxLines="2"


Same previous comment applies to this code too.

Remove maxLines attribute.

Also, please re-generate screenshots and update PR description just to be sure

KevinGitonga · 2022-09-13T06:46:42Z

@rt4914 have made changes. For phone screenshots they are similar to those shared and for the tablet also they remain as i had updated. This is after checking on my device and tablet emulator on both Portrait and Landscape. PTAL.

rt4914

LGTM, Thanks.

rt4914 · 2022-09-14T19:45:34Z

@KevinGitonga CI checks are failing. Have a look.

BenHenning

Thanks @KevinGitonga! This also LGTM.

BenHenning · 2022-09-16T08:35:34Z

Except one thing @KevinGitonga: please make sure to update the title to include the bug being fixed (as I edited just now) for future PRs.

KevinGitonga · 2022-09-16T09:46:41Z

Except one thing @KevinGitonga: please make sure to update the title to include the bug being fixed (as I edited just now) for future PRs.

Thank's @BenHenning well noted.

Reduce MarginStart and MarginEnd.

3a96740

KevinGitonga requested a review from rt4914 as a code owner September 1, 2022 11:41

KevinGitonga assigned BenHenning Sep 1, 2022

BenHenning assigned KevinGitonga and unassigned BenHenning Sep 1, 2022

Update maxLines to automatically to show extra text on new line.

d6e03cc

KevinGitonga assigned BenHenning and unassigned KevinGitonga Sep 2, 2022

BenHenning assigned rt4914 and KevinGitonga and unassigned BenHenning Sep 5, 2022

Update maxLines for other portrait layouts.

dbbae53

KevinGitonga removed their assignment Sep 5, 2022

rt4914 suggested changes Sep 7, 2022

View reviewed changes

rt4914 assigned KevinGitonga and unassigned rt4914 Sep 7, 2022

KevinGitonga changed the title ~~Reduce MarginStart and MarginEnd.~~ Remove and Update maxLines contraint's to enable lengthy text to linebreak. Sep 8, 2022

KevinGitonga assigned rt4914 and unassigned KevinGitonga Sep 8, 2022

Make changes as requested on review.

b370af2

KevinGitonga changed the title ~~Remove and Update maxLines contraint's to enable lengthy text to linebreak.~~ Remove and Update maxLines contsraint's to enable lengthy text to linebreak. Sep 8, 2022

rt4914 suggested changes Sep 12, 2022

View reviewed changes

rt4914 assigned KevinGitonga and unassigned rt4914 Sep 12, 2022

Make more changes as advised by reviewer.

c391b54

KevinGitonga removed their assignment Sep 13, 2022

KevinGitonga assigned rt4914 Sep 13, 2022

rt4914 approved these changes Sep 14, 2022

View reviewed changes

rt4914 assigned KevinGitonga and unassigned rt4914 Sep 14, 2022

oppiabot bot added the PR: LGTM label Sep 14, 2022

Merge branch 'oppia:develop' into fix_portuguese_text_cut_off

39c99a0

KevinGitonga changed the title ~~Remove and Update maxLines contsraint's to enable lengthy text to linebreak.~~ Remove maxLines contsraint's to enable long text to linebreak. Sep 15, 2022

KevinGitonga assigned BenHenning and unassigned KevinGitonga Sep 15, 2022

BenHenning approved these changes Sep 16, 2022

View reviewed changes

BenHenning changed the title ~~Remove maxLines contsraint's to enable long text to linebreak.~~ Fix #3822: Remove maxLines contsraint's to enable long text to linebreak Sep 16, 2022

BenHenning merged commit 1eebfb5 into oppia:develop Sep 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #3822: Remove maxLines contsraint's to enable long text to linebreak #4547

Fix #3822: Remove maxLines contsraint's to enable long text to linebreak #4547

KevinGitonga commented Sep 1, 2022 •

edited by BenHenning

Loading

BenHenning commented Sep 1, 2022

KevinGitonga commented Sep 2, 2022

BenHenning commented Sep 5, 2022

rt4914 left a comment

rt4914 Sep 7, 2022

KevinGitonga Sep 8, 2022

KevinGitonga commented Sep 8, 2022

rt4914 left a comment

rt4914 Sep 12, 2022

KevinGitonga Sep 13, 2022

rt4914 Sep 12, 2022

KevinGitonga Sep 13, 2022

rt4914 Sep 12, 2022

KevinGitonga Sep 13, 2022

KevinGitonga commented Sep 13, 2022 •

edited

Loading

rt4914 left a comment

rt4914 commented Sep 14, 2022

BenHenning left a comment

BenHenning commented Sep 16, 2022

KevinGitonga commented Sep 16, 2022

Fix #3822: Remove maxLines contsraint's to enable long text to linebreak #4547

Fix #3822: Remove maxLines contsraint's to enable long text to linebreak #4547

Conversation

KevinGitonga commented Sep 1, 2022 • edited by BenHenning Loading

Explanation

Essential Checklist

For UI-specific PRs only

Phone screenshots

Tab screenshots

BenHenning commented Sep 1, 2022

KevinGitonga commented Sep 2, 2022

BenHenning commented Sep 5, 2022

rt4914 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinGitonga commented Sep 8, 2022

rt4914 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

KevinGitonga commented Sep 13, 2022 • edited Loading

rt4914 left a comment

Choose a reason for hiding this comment

rt4914 commented Sep 14, 2022

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning commented Sep 16, 2022

KevinGitonga commented Sep 16, 2022

KevinGitonga commented Sep 1, 2022 •

edited by BenHenning

Loading

KevinGitonga commented Sep 13, 2022 •

edited

Loading