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

Home Topic Card UI Regression #2749

Closed
rt4914 opened this issue Feb 22, 2021 · 7 comments · Fixed by #5185
Closed

Home Topic Card UI Regression #2749

rt4914 opened this issue Feb 22, 2021 · 7 comments · Fixed by #5185
Assignees
Labels
bug End user-perceivable behaviors which are not desirable. 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). Issue: Needs Clarification Indicates that an issue needs more detail in order to be able to be acted upon. Work: Low Solution is clear and broken into good-first-issue-sized chunks. Z-ibt Temporary label for Ben to keep track of issues he's triaged.

Comments

@rt4914
Copy link
Contributor

rt4914 commented Feb 22, 2021

The Home screen topic cards look weird in various devices and that's because of following reasons:

  1. Span count is not dynamic.
  2. If the extreme end margins of the screen is large then it's because of span count and the cards look really bad. Because span count divides the entire screen in equal parts.
  3. We have to keep the inner margins consistent too which is not easy with span count.

Overall we are keeping the extreme margins and span count constant and the width of the topic cards is dynamic and the inner margins are dynamic. Because of all these conditions the UI looks bad.

Some References:

https://drive.google.com/drive/folders/1SLQVZD5FSJoZASWwpLNrkC_InQiq_61t?usp=sharing
Nexus 7 Device:

@rt4914 rt4914 modified the milestones: Backlog, Beta Feb 22, 2021
@rt4914 rt4914 added Priority: Essential This work item must be completed for its milestone. Type: Bug labels Feb 22, 2021
@rt4914
Copy link
Contributor Author

rt4914 commented Feb 22, 2021

@BenHenning PTAL

@Akshat-gour
Copy link
Contributor

Akshat-gour commented Mar 2, 2021

@rt4914
is it available
can i work on it

@rt4914
Copy link
Contributor Author

rt4914 commented Mar 3, 2021

@Akshat-gour Apologies for inconvenience. I will assign it to me and Ben.

@sakets3010
Copy link

@rt4914 i would like to work upon this, if its still available. Thanks!

@rt4914
Copy link
Contributor Author

rt4914 commented Mar 22, 2021

@sakets3010 This is not a good-first-issue. Suggest solving few good-first-issues before taking harder issues.

@BenHenning BenHenning added Z-ibt Temporary label for Ben to keep track of issues he's triaged. issue_user_learner labels Sep 15, 2022
@BenHenning BenHenning removed this from the Beta MR2 milestone Sep 16, 2022
@seanlip seanlip added bug End user-perceivable behaviors which are not desirable. and removed issue_user_learner labels Mar 29, 2023
@adhiamboperes adhiamboperes added the Work: Low Solution is clear and broken into good-first-issue-sized chunks. label Jul 16, 2023
@adhiamboperes adhiamboperes added good first issue This item is good for new contributors to make their pull request. and removed Priority: Essential This work item must be completed for its milestone. labels Sep 25, 2023
@adhiamboperes adhiamboperes added this to the 1.0 Global availability milestone Sep 25, 2023
@theMr17 theMr17 self-assigned this Oct 10, 2023
@adhiamboperes adhiamboperes added the Issue: Needs Clarification Indicates that an issue needs more detail in order to be able to be acted upon. label Oct 20, 2023
@BenHenning
Copy link
Member

Per your question in #5185 @adhiamboperes, I think the problem here is that the way the home screen lays out the list of topics today doesn't work well on all device configurations. We use a fixed span count and static tile sizes which means certain devices are going to result in too small or too large margins between the tiles, or tiles that are too small. We partially manage this by increasing the fixed number of tiles to show or tile sizes for certain configurations (using layout qualifiers), but per the picture in the OP this isn't a perfect, device-wide solution.

A more flexible solution might be to instead:

  1. Revisit tile sizes across device configurations to ensure they're a reasonable size on all configurations (and not too small as shown in the screenshot above).
  2. Dynamically compute the number of tiles to show based on screen size, tile size, and required minimum margins between tiles.

(2) I think is the hard part to solve here, but we may not need to solve it if we can make (1) much better. If the tiles are larger targets for tapping and they look like they better fill their available space, then (2) might be less of an issue.

I also want to clarify one thing: Rajat and I spent a lot of time discussing ways of making both the tile size and tile counts dynamic based on screen size, but we decided it was too complicated. Instead, it's much more approachable to make one dynamic and the other static (however both are static today as mentioned above, and since that's the simplest solution we probably should stay with that unless there's some reason we can't solve (1) above).

@adhiamboperes
Copy link
Collaborator

@theMr17, Ben and I discussed this further, and here are some notes to improve your PR for this issue:

Re:

Revisit tile sizes across device configurations to ensure they're a reasonable size on all configurations (and not too small as shown in the screenshot above).

Since the tiles are fixed size, and the margins are minimum sized, we don’t want them too close together.

  • The exercise is about having better tile sizing for different device configurations. We can pick different device densities and see how the tiles look at devices at the small and large ends of the spectrum within that display density profile, from mdpi to xxxhdpi, and tweak it for that density if it is too small or too large. This is about 12 different configs. We can most easily customize for hdpi, but we want to check the boundaries and come up with a value that looks nice on all the devices.
  • It is important to make sure that tablet still looks good. We can pick a smaller and larger tablet to double check they both look okay.
  • Do take screenshots of the before and after of all these cases.
  • Even though the tile size is the main thing to configure here, feel free to tweak the other dimensions like margins to make it look right.

adhiamboperes added a commit that referenced this issue Feb 19, 2024
<!-- READ ME FIRST: Please fill in the explanation section below and
check off every point from the Essential Checklist! -->
## Explanation
<!--
- Explain what your PR does. If this PR fixes an existing bug, please
include
- "Fixes #bugnum:" in the explanation so that GitHub can auto-close the
issue
  - when this PR is merged.
  -->

Fixes #2749

Modifies margin and padding values

## 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
<!-- 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))
- For PRs introducing new UI elements or color changes, both light and
dark mode screenshots must be included
- Add a screenshot demonstrating that you ran affected Espresso tests
locally & that they're passing

## Tablet Screenshots

### 1920 x 1200
|Before|After|
|--|--|
|![1920 x 1200 port
before](https://github.com/oppia/oppia-android/assets/84731134/dbfc50df-4ced-48f9-85bd-11ed2ca9d60f)|![1920
x 1200 port
after](https://github.com/oppia/oppia-android/assets/84731134/ef208183-3198-442b-958e-7d2f85a37b9f)|
|![1920 x 1200
before](https://github.com/oppia/oppia-android/assets/84731134/344d0ef7-1fac-4117-9adb-b85877a40b3f)|![1920
x 1200
after](https://github.com/oppia/oppia-android/assets/84731134/fa8faa1c-9337-4741-86d1-32a655275768)|

### 2560 x 1800
|Before|After|
|--|--|
|![2560 x 1800 port
before](https://github.com/oppia/oppia-android/assets/84731134/700abf1e-951f-42a3-a465-1ee6852685e6)|![2560
x 1800 port
after](https://github.com/oppia/oppia-android/assets/84731134/b34f72ae-0d0b-4e67-af54-cf9bf292036f)|
|![2560 x 1800
before](https://github.com/oppia/oppia-android/assets/84731134/f05fee95-9d8d-4310-a9e0-c8223b2c4ed4)|![2560
x 1800
after](https://github.com/oppia/oppia-android/assets/84731134/a9a2cb09-bcd8-4b50-888c-fdfa89d23274)|

### 1024 x 600
|Before|After|
|--|--|
|![1024 x 600
before](https://github.com/oppia/oppia-android/assets/84731134/59f77ffd-5ac6-46e9-8c20-33287848acc1)|![1024
x 600
after](https://github.com/oppia/oppia-android/assets/84731134/bdc14f4a-9656-4549-8e98-28535c214a71)|

## Phone Screenshots

|dpi|Screenshots|
|--|--|

|ldpi|![image](https://github.com/oppia/oppia-android/assets/84731134/eae24f4d-5245-4f23-8b22-734b6c363c95)|

|mdpi|![image](https://github.com/oppia/oppia-android/assets/84731134/d38ef410-8cb1-4225-bb43-532db99d483f)|

|hdpi|![image](https://github.com/oppia/oppia-android/assets/84731134/1dd8ae92-1fc8-4ece-827e-0ad09cf06e0e)|

|xhdpi|![image](https://github.com/oppia/oppia-android/assets/84731134/01f7f665-c2e2-4e70-b679-2d81f5b18b07)![image](https://github.com/oppia/oppia-android/assets/84731134/5f0499ff-9bcd-4ec2-8d32-685420feabe4)|

|xxhdpi|![image](https://github.com/oppia/oppia-android/assets/84731134/dc727cd4-eb60-4736-aab1-e280a33933df)![image](https://github.com/oppia/oppia-android/assets/84731134/3baebd40-402c-4fe0-a7cd-5c339bf786b2)|

---------

Co-authored-by: Adhiambo Peres <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug End user-perceivable behaviors which are not desirable. 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). Issue: Needs Clarification Indicates that an issue needs more detail in order to be able to be acted upon. Work: Low Solution is clear and broken into good-first-issue-sized chunks. Z-ibt Temporary label for Ben to keep track of issues he's triaged.
Development

Successfully merging a pull request may close this issue.

8 participants