Fix #4361: Continue button is not aligned properly when reading text size in extra large #4385

Ryggs · 2022-06-08T00:11:57Z

Explanation

Fixes #4361 Continue button is not aligned properly when reading text size is extra large

This pr fixes :--

Button size for continue button overlapping the text 'continue' and causing a break word on the button while text size is set to extra Large.

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](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
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](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

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

Changes made

Device Type	Before	After	Landscape & Potrait
Handset
Tablet

Demo Video

fix_continue_button_issue.mp4

github-actions · 2022-06-08T00:12:15Z

Hi! @Ryggs Welcome to Oppia! Could you please follow the instructions here and sign the CLA Sheet to get started? You'll need to do this before we can accept your PR. Thanks!

github-actions · 2022-06-08T01:44:42Z

Hi! @Ryggs Welcome to Oppia! Could you please follow the instructions here and sign the CLA Sheet to get started? You'll need to do this before we can accept your PR. Thanks!

BenHenning · 2022-06-08T02:02:22Z

Thanks @Ryggs! There are some aspects of this PR that should be changed:

The PR title needs to include the issue being fixed (i.e. Continue button is not aligned properly when reading text size is extra large #4361).
The PR description should use a filled in version of the template (don't actually erase it, though the comments can be erased)
The included video is really good. I suggest changing your screenshots such that:
- They show the before & after state
- They show landscape & portrait
- They show handset & tablet layouts (this one is particularly interesting since tablets often use different dimension sizes)
- They show large, medium, and small zoom
I recommend putting the above in a tablet since 24 images might otherwise make the PR description a bit large to view (see Fix #4044: Introduce UI support for math expressions & new interactions #2173 for a good reference of this).
The PR description should include details about what the problem is, what the fix is, how it works, and why it's the best choice (plus any other considerations that you think are relevant for the reviewer to know).
All functional changes in the codebase should include test changes (with the exception being a few special cases that can't be tested), so please make sure to update/add tests as necessary to cover this change.
Will your fix work in all cases? Have you tried maximizing display and font zoom at the system level (rather than in-app) to see how the continue button behaves? I'm wondering if a static DP size is actually going to work here, or if something more dynamic needs to be done. (@rt4914 feel free to pipe in if you have any thoughts or suggestions).

rt4914

@Ryggs PTAL, Thanks.

rt4914 · 2022-06-08T19:47:05Z

app/src/main/res/values/dimens.xml

@@ -377,7 +377,7 @@
  <dimen name="general_button_item_split_view_margin_top_left">20dp</dimen>
  <dimen name="general_button_item_split_view_margin_top_right">68dp</dimen>
  <dimen name="general_button_item_margin_top">20dp</dimen>
-  <dimen name="continue_submit_button_width">88dp</dimen>
+  <dimen name="continue_submit_button_width">180dp</dimen>


I think we should change this behaviour.

Currently we are keeping the width of button to be fixed and for large fonts we have increased that to 180dp but this will start creating issues for smaller font settings.

Also ideally the width should be wrap_content and other attributes like margin, padding, fontSize should handle the UI.

For this specific case following changes to the Continue Button should mostly fix the issue:

Instead of fixed width as 180dp we should make layout_width as wrap_content.

We should keep a padding of 10dp as per mocks - https://xd.adobe.com/view/3dca36c2-5115-419c-b25e-0f10526b077c-6899/screen/0fe704c1-f8c7-4abc-8b04-2a0ed1285db4/

To test this fully make sure you generate screenshots of small, medium as well as large font size.
You might need to look at this link about how to use mocks correctly: https://github.com/oppia/oppia-android/wiki/Working-on-UI

@Ryggs when addressing comments, please reply rather than marking them as resolved (generally, only the origin comment author should resolve their comment since they need to check that you've addressed the original point, and it's often easy to miss comments that were closed by other people).

That being said, has @rt4914's comment been fully addressed?

@rt4914 when you say we replace the 180dp dimension and make the layout_width as wrap_content, should this be in the dimens folder replacing the "continue_submit_button_width" ?
The current Padding is 88dp and changing it to 188dp accomodates all screen sizes. Take a look at the updated comment on the PR. Thanks

@Ryggs 188dp might work for some screens or maybe all but it is actually not the correct way.
The approach should be to avoid giving fixed widths to buttons.

I suggest removing continue_submit_button_width fully and instead using wrap_content in the layout file where this button has been created.
Also we should keep a padding of 10dp as per mocks - https://xd.adobe.com/view/3dca36c2-5115-419c-b25e-0f10526b077c-6899/screen/0fe704c1-f8c7-4abc-8b04-2a0ed1285db4/

To test this fully make sure you generate screenshots of small, medium as well as large font size.
You might need to look at this link about how to use mocks correctly: https://github.com/oppia/oppia-android/wiki/Working-on-UI

@Ryggs Assign it back to me whenever you need a reply.

Hello Rajat, this is the error I am getting after replacing continue_submit_button_width fully and instead using wrap_content in the layout file

THis is how I did it.

Replaced the line calling the max_width dimension and replaced with app_layout_width and used wrap_content as that is the only way to do it perhaps?
I'll appreciate your feedback PTAL

@Ryggs I believe attributes are case sensitive. It seems like you might've unintentionally capitalized the 'w' for width. Plus, shouldn't this be an android not an app attribute? E.g. android:layout_width.

Let me try this out now

@Ryggs You can remove this value from all dimens now as it might not be getting used.

Hi @rt4914 I have removed this value. PTAL

oppiabot · 2022-06-15T20:02:52Z

Hi @Ryggs, 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!

Chandra-Sekhar-Bala · 2022-06-20T16:05:44Z

Hey @rt4914 can I work on this?

oppiabot · 2022-06-27T16:59:46Z

Hi @Ryggs, 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-07-05T20:42:52Z

Hi @Ryggs, 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-07-27T21:26:21Z

Hi @Ryggs, 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!

Ryggs

Updated fix from using maxWitdth to wrap_content att for Continue Button

Ryggs · 2022-08-08T03:41:15Z

@Ryggs PTAL, Thanks.

PTAL for a review @rt4914

rt4914

@Ryggs PTAL

rt4914 · 2022-08-08T18:56:19Z

app/src/main/res/values/dimens.xml

@@ -377,7 +377,7 @@
  <dimen name="general_button_item_split_view_margin_top_left">20dp</dimen>
  <dimen name="general_button_item_split_view_margin_top_right">68dp</dimen>
  <dimen name="general_button_item_margin_top">20dp</dimen>
-  <dimen name="continue_submit_button_width">88dp</dimen>
+  <dimen name="continue_submit_button_width">180dp</dimen>


@Ryggs You can remove this value from all dimens now as it might not be getting used.

app/src/main/res/values/dimens.xml

Ryggs · 2022-08-11T11:50:11Z

Hi @rt4914 I made the requested changes. PTAL

rt4914

LGTM, Thanks.

Ryggs added 2 commits June 8, 2022 02:47

fixed_continue_action_button

83d4320

fixed_continue_action_button

94cd275

Ryggs requested a review from rt4914 as a code owner June 8, 2022 00:11

seanlip assigned rt4914 and BenHenning Jun 8, 2022

seanlip closed this Jun 8, 2022

seanlip reopened this Jun 8, 2022

seanlip closed this Jun 8, 2022

seanlip reopened this Jun 8, 2022

BenHenning assigned Ryggs and unassigned BenHenning Jun 8, 2022

Ryggs changed the title ~~Fix continue button issue~~ Fix #4361: Continue button is not aligned properly when reading text size is extra large #4361 Jun 8, 2022

Ryggs changed the title ~~Fix #4361: Continue button is not aligned properly when reading text size is extra large #4361~~ Fix #4361: Continue button is not aligned properly when reading text size is extra large Jun 8, 2022

rt4914 suggested changes Jun 8, 2022

View reviewed changes

rt4914 removed their assignment Jun 8, 2022

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

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

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

Ryggs requested a review from BenHenning June 28, 2022 20:02

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

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

Ryggs assigned BenHenning Jul 8, 2022

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

BenHenning assigned Ryggs and unassigned BenHenning Jul 12, 2022

Ryggs changed the title ~~Fix #4361: Continue button is not aligned properly when reading text size is extra large~~ Fix #4361: Continue button is not aligned properly when reading text size in extra large Jul 13, 2022

Ryggs assigned rt4914 Jul 18, 2022

rt4914 removed their assignment Jul 18, 2022

BenHenning mentioned this pull request Jul 20, 2022

Fixes #4361 - Continue button is now aligned properly when reading text size is extra large #4402

Closed

6 tasks

oppiabot bot added stale Corresponds to items that haven't seen a recent update and may be automatically closed. and removed stale Corresponds to items that haven't seen a recent update and may be automatically closed. labels Jul 27, 2022

Ryggs assigned rt4914 and unassigned Ryggs Jul 29, 2022

Ryggs reopened this Aug 1, 2022

rt4914 assigned Ryggs and unassigned rt4914 Aug 2, 2022

add layout width

ff953c4

Ryggs commented Aug 8, 2022

View reviewed changes

Ryggs removed their assignment Aug 8, 2022

Ryggs requested a review from rt4914 August 8, 2022 03:39

Ryggs assigned rt4914 Aug 8, 2022

rt4914 suggested changes Aug 8, 2022

View reviewed changes

rt4914 assigned Ryggs and unassigned rt4914 Aug 8, 2022

removed continue_submit_button_width dimens

a1784b0

Ryggs assigned rt4914 and unassigned Ryggs Aug 10, 2022

rt4914 approved these changes Aug 13, 2022

View reviewed changes

rt4914 merged commit a55c8b2 into oppia:develop Aug 13, 2022

BenHenning mentioned this pull request Aug 22, 2022

No enough padding on Continue button style #4504

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4361: Continue button is not aligned properly when reading text size in extra large #4385

Fix #4361: Continue button is not aligned properly when reading text size in extra large #4385

Ryggs commented Jun 8, 2022 •

edited by BenHenning

Loading

github-actions bot commented Jun 8, 2022

github-actions bot commented Jun 8, 2022

BenHenning commented Jun 8, 2022

rt4914 left a comment

rt4914 Jun 8, 2022

BenHenning Jul 12, 2022 •

edited

Loading

Ryggs Jul 14, 2022

rt4914 Jul 18, 2022

rt4914 Jul 18, 2022

Ryggs Jul 29, 2022

BenHenning Jul 29, 2022

Ryggs Aug 1, 2022

rt4914 Aug 8, 2022

Ryggs Aug 10, 2022

oppiabot bot commented Jun 15, 2022

Chandra-Sekhar-Bala commented Jun 20, 2022

oppiabot bot commented Jun 27, 2022

oppiabot bot commented Jul 5, 2022

oppiabot bot commented Jul 27, 2022

Ryggs left a comment

Ryggs commented Aug 8, 2022 •

edited

Loading

rt4914 left a comment

rt4914 Aug 8, 2022

Ryggs commented Aug 11, 2022

rt4914 left a comment

Fix #4361: Continue button is not aligned properly when reading text size in extra large #4385

Fix #4361: Continue button is not aligned properly when reading text size in extra large #4385

Conversation

Ryggs commented Jun 8, 2022 • edited by BenHenning Loading

Explanation

Essential Checklist

For UI-specific PRs only

Changes made

Demo Video

github-actions bot commented Jun 8, 2022

github-actions bot commented Jun 8, 2022

BenHenning commented Jun 8, 2022

rt4914 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenHenning Jul 12, 2022 • edited Loading

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

oppiabot bot commented Jun 15, 2022

Chandra-Sekhar-Bala commented Jun 20, 2022

oppiabot bot commented Jun 27, 2022

oppiabot bot commented Jul 5, 2022

oppiabot bot commented Jul 27, 2022

Ryggs left a comment

Choose a reason for hiding this comment

Ryggs commented Aug 8, 2022 • edited Loading

rt4914 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ryggs commented Aug 11, 2022

rt4914 left a comment

Choose a reason for hiding this comment

Ryggs commented Jun 8, 2022 •

edited by BenHenning

Loading

BenHenning Jul 12, 2022 •

edited

Loading

Ryggs commented Aug 8, 2022 •

edited

Loading