Skip to content
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 UI for profile chooser screen in add-view #2116

Closed
rt4914 opened this issue Nov 16, 2020 · 23 comments · Fixed by #4965
Closed

Fix UI for profile chooser screen in add-view #2116

rt4914 opened this issue Nov 16, 2020 · 23 comments · Fixed by #4965
Assignees
Labels
enhancement End user-perceivable enhancements. good first issue This item is good for new contributors to make their pull request. Impact: Low Low perceived user impact (e.g. edge cases). Priority: Essential This work item must be completed for its milestone. Z-ibt Temporary label for Ben to keep track of issues he's triaged.

Comments

@rt4914 rt4914 added Type: Improvement Priority: Essential This work item must be completed for its milestone. good first issue This item is good for new contributors to make their pull request. labels Nov 16, 2020
@rt4914 rt4914 added this to the Beta milestone Nov 16, 2020
@ShridharGoel
Copy link
Contributor

I would like to work on this.

@ShridharGoel
Copy link
Contributor

ShridharGoel commented Nov 22, 2020

This is how the screens look currently:

Select Profile 1

Select Profile 2

Since it looks in accordance with the designs, may I know what needs to be changed?

@rt4914
Copy link
Contributor Author

rt4914 commented Nov 23, 2020

This is how the screens look currently:

Select Profile 1 Select Profile 2

Since it looks in accordance with the designs, may I know what needs to be changed?

@ShridharGoel If you run it on latest develop and Nexus & tablet. You will see following UI. Where the Add Profile is not aligned.
Screenshot_1606137020

@ShridharGoel
Copy link
Contributor

@rt4914 Yes, the issue is replicable on the latest commits. Thanks.

ShridharGoel added a commit to ShridharGoel/oppia-android that referenced this issue Nov 24, 2020
ShridharGoel added a commit to ShridharGoel/oppia-android that referenced this issue Nov 24, 2020
ShridharGoel added a commit to ShridharGoel/oppia-android that referenced this issue Nov 24, 2020
ShridharGoel added a commit to ShridharGoel/oppia-android that referenced this issue Nov 24, 2020
@rt4914 rt4914 assigned rt4914 and unassigned ShridharGoel Dec 29, 2020
@rt4914 rt4914 added this to Needs Triage in CLAM Team (deprecated -- please use new board) via automation Jul 12, 2021
@rt4914 rt4914 moved this from Needs Triage to Beta (Bugs & Fixes) in CLAM Team (deprecated -- please use new board) Jul 12, 2021
@rt4914 rt4914 moved this from Beta (Bugs & Fixes) to High-fi Beta in CLAM Team (deprecated -- please use new board) Jul 12, 2021
@rt4914 rt4914 removed their assignment Jan 18, 2022
@BenHenning BenHenning modified the milestones: Beta, Beta MR1 Jun 11, 2022
@adhiamboperes
Copy link
Collaborator

Unassigning @ArchitJain1201 due to inactivity

@seanlip seanlip added enhancement End user-perceivable enhancements. and removed issue_user_learner labels Mar 29, 2023
@aayushimathur6
Copy link
Collaborator

@adhiamboperes as per the expected issue, when I m reproducing it mine is coming a bit aligned. PTAL
tablet

@adhiamboperes
Copy link
Collaborator

adhiamboperes commented Apr 17, 2023

@adhiamboperes as per the expected issue, when I m reproducing it mine is coming a bit aligned. PTAL !

Hi @aayushimathur6, can you share the specifications of the device you used to repro? One tip is to use an emulator with the same specifications as the device on which the issue was reported.

@adhiamboperes
Copy link
Collaborator

adhiamboperes commented Apr 18, 2023

@adhiamboperes as per the expected issue, when I m reproducing it mine is coming a bit aligned. PTAL !

Hi @aayushimathur6, can you share the specifications of the device you used to repro? One tip is to use an emulator with the same specifications as the device on which the issue was reported.

I attempted to repro the issue myself on the Nexus 9 API 29 and I can confirm that it is no longer reproducing, additionally, the recent work done in #4849 and #4844 contain screenshots that confirm this.

Tablet Landscape
Screenshot 2023-04-18 at 10 08 58

Tablet Portrait
Screenshot 2023-04-18 at 10 10 04

@adhiamboperes
Copy link
Collaborator

However, subsequent work in #4874 breaks the default profile chooser view.

Screenshot_1681801427

@seanlip, what's the appropriate action in this case between creating a new issue for this and reverting #4874?

@seanlip
Copy link
Member

seanlip commented Apr 18, 2023

If the issue was caused by #4874 (i.e. it worked fine before that PR and didn't after that PR) then please revert that PR. That's better than trying to fix-forward (unless the fix is obvious) because develop should always be clean and releasable.

Thanks for checking!

@adhiamboperes
Copy link
Collaborator

If the issue was caused by #4874 (i.e. it worked fine before that PR and didn't after that PR) then please revert that PR. That's better than trying to fix-forward (unless the fix is obvious) because develop should always be clean and releasable.

Thanks for checking!

I do not have the appropriate permissions to revert this so I will leave it for @BenHenning.

Screenshot 2023-04-18 at 12 03 08

@adhiamboperes
Copy link
Collaborator

adhiamboperes commented Apr 18, 2023

Otherwise we can close this issue as fixed. Thanks @aayushimathur6

@aayushimathur6
Copy link
Collaborator

Otherwise we can close this issue as fixed. Thanks @aayushimathur6

sure, Thanks @adhiamboperes I will find a new one to work on

@aayushimathur6
Copy link
Collaborator

@adhiamboperes as per the expected issue, when I m reproducing it mine is coming a bit aligned. PTAL !

Hi @aayushimathur6, can you share the specifications of the device you used to repro? One tip is to use an emulator with the same specifications as the device on which the issue was reported.

Nexus 9 API 29

@adhiamboperes adhiamboperes removed the Issue: Needs Clarification Indicates that an issue needs more detail in order to be able to be acted upon. label Apr 20, 2023
BenHenning pushed a commit that referenced this issue Apr 21, 2023
…dableAdapter" (#4951)

This reverts commit `7fd290d68f0440d926f2d443dbd7bfb28ab20547`, fixing
part of #2116.

While testing #2116, we found that the UI for default profile view does
not display correctly, and traced the change to #4874 in [this
discusson](#2116 (comment)).

#4874 introduces a change to `MarginBindingAdapters` which utilizes
`MarginLayoutParamsCompat` to compute the margins of a layout before
drawing it. I suspect that this works for the `Promoted Story` and
`Topic Summary` because we compute the start and end margins of each
item relative to grid columns laid out on the HomeActivity, but no such
implementation exists for the Profile Chooser view. We need to do some
further investigation into this, hence the decision to revert.

We will need to test all the associated screens to make sure they still
look as expected:

![Screenshot 2023-04-19 at 18 01
28](https://user-images.githubusercontent.com/59600948/233117952-8c981fa2-2d0c-45cc-80ff-651862c1fd96.png)


## Essential Checklist
<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [x] 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: ...".)
- [x] Any changes to
[scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
files have their rationale included in the PR explanation.
- [x] The PR follows the [style
guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] 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)).
- [x] The PR is made from a branch that's **not** called "develop" and
is up-to-date with "develop".
- [x] 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
This was mainly a re-implimentation per #632, and the UI did not change.
Please refer to #4874 for screenshots.

The Affected screen displays okay with this revert:


![Screenshot_1681916915](https://user-images.githubusercontent.com/59600948/233125096-2242fb7e-eb9f-4b78-8db0-c22abb6b5cc3.png)

Other screens:
|   |   |
|---|---|

|![Screenshot_1681917117](https://user-images.githubusercontent.com/59600948/233126283-45a860c7-26ac-47f6-8b38-e54b1ee7cb1f.png)|![Screenshot_1681917190](https://user-images.githubusercontent.com/59600948/233126350-08033853-4434-462d-9bf3-98040ff2356d.png)|
@masclot
Copy link
Collaborator

masclot commented Apr 28, 2023

I tried to reproduce this in Nexus 9 emulator, API 29, but couldn't. I tried with the old branch I still have with the original changes, and a new branch where I only apply the changes to MarginBindingAdapters. I could not reproduce the issue.
Screenshots:

landscape portrait
profile_chooser_landscape profile_chooser_portrait

Did you change any configuration?

@adhiamboperes
Copy link
Collaborator

Hi @masclot, I have confirmed that I can repro this when I check out commit 7fd290d68f0440d926f2d443dbd7bfb28ab20547, which is the one I reverted. I suspect that your view may be cached. Can your clear your cache and retry?
I did not change any configs--this is a clean install of the app.

@masclot
Copy link
Collaborator

masclot commented May 5, 2023

Thanks @adhiamboperes I could reproduce the issue by clearing the application data, which shows a somewhat different profile chooser on app start. Also, I could only reproduce in landscape mode.

adhiamboperes added a commit that referenced this issue Jun 24, 2023
…es. Fix #2116: UI misalignment due to not resetting the layout. (#4965)

## Explanation

Fix #2116:
Fix part of #632:

Simplify MarginBindingAdapters.

The code is mostly the same as in PR #4874, which was reverted. This PR
fixes the bug that caused the rollback.
The rest of the reverted changes in PR #4874 will be included in a
separate PR.

The fix consists of resetting the layout params after changing the top
and bottom margins, by calling View.setLayoutParams(), as explained in
the [layout params
documentation](https://developer.android.com/reference/android/view/ViewGroup.MarginLayoutParams#bottomMargin).
The same call is not needed when calling a method instead of setting a
property.

It also fixes an RTL issue: if the view is not attached yet, there is no
layout direction information available, making the old implementation
render in LTR always.

Some screenshots:

|new|current|
|-|-|

|![home_rtl](https://user-images.githubusercontent.com/103062089/236451422-13843234-5f6c-4ed3-997a-e4b0459c3641.png)
|
![home_rtl_current](https://user-images.githubusercontent.com/103062089/236451614-55595920-ed58-4329-8d7d-7bdbc0487688.png)|

|![profile_chooser_landscape](https://user-images.githubusercontent.com/103062089/236451425-6766ed39-4a6b-4918-bf62-28bff5d05565.png)|![profile_chooser_landscape_current](https://user-images.githubusercontent.com/103062089/236451616-6979857b-f431-45cf-82c1-8db2a7618669.png)|

|![profile_chooser_portrait](https://user-images.githubusercontent.com/103062089/236451427-ba5cc671-ff03-4718-ae45-9e3cc924efeb.png)|![profile_chooser_portrait_current](https://user-images.githubusercontent.com/103062089/236451621-058ec5b9-0cdb-4713-931e-c49bd9a5edba.png)|

|![recently_played_rtl](https://user-images.githubusercontent.com/103062089/236451431-01e65a32-b448-422d-88a1-efb5326d5ab9.png)|![recently_played_rtl_current](https://user-images.githubusercontent.com/103062089/236451622-57a5cc80-f0d9-400d-8003-e9ff9735950b.png)|

Test results:
![Test
results](https://user-images.githubusercontent.com/103062089/236451433-d5a0e34f-75c0-4a6d-aeb5-88012913796f.png)


## Essential Checklist
<!-- Please tick the relevant boxes by putting an "x" in them. -->
- [x] 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: ...".)
- [x] Any changes to
[scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
files have their rationale included in the PR explanation.
- [x] The PR follows the [style
guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] 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)).
- [x] 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
<!-- Delete these section if this PR does not include UI-related
changes. -->
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](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines))
- Add a video showing the full UX flow with a screen reader enabled (see
[accessibility
guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide))
- Add a screenshot demonstrating that you ran affected Espresso tests
locally & that they're passing

---------

Co-authored-by: Adhiambo Peres <59600948+adhiamboperes@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement End user-perceivable enhancements. good first issue This item is good for new contributors to make their pull request. Impact: Low Low perceived user impact (e.g. edge cases). Priority: Essential This work item must be completed for its milestone. Z-ibt Temporary label for Ben to keep track of issues he's triaged.