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.

Sign up for GitHub

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

Jump to bottom

Fix #4449: Modify the design of the chapter list in lessons tab #4535

Merged
merged 42 commits into from
Sep 20, 2022
Merged

Fix #4449: Modify the design of the chapter list in lessons tab #4535

merged 42 commits into from
Sep 20, 2022

Conversation

JishnuGoyal
Copy link
Contributor

@JishnuGoyal JishnuGoyal commented Aug 25, 2022 •
edited
Loading

Explanation

###Quick before an after preview:

image

This PR, as part of the GSoC project: Interactive Onboarding Flow, modifies the look of the recycler view adapter items on the topic lessons tab.
Now there are different looking item view holders for completed, in-progress, locked and not-started chapter which results in a clear CTA for the users, and they are more likely to click on the correct option. Link to the UI mock: https://xd.adobe.com/view/3dca36c2-5115-419c-b25e-0f10526b077c-6899/screen/558af437-bec4-4719-8d30-291da10bbc9d/specs/

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

Before:
image

image

After:
image

image

image

image

Tablet View: (Pixel C API 30)
Portrait
image
image

Landscape
image

with green border:
image

Accessibility scanner output:
The background to foreground ratio of these view have been kept this way on-purpose to let the users know that those chapters are locked.
image

image

RTL:
image

image

Talkback:

WhatsApp.Video.2022-09-18.at.1.54.28.AM.1.mp4

@JishnuGoyal JishnuGoyal requested a review from rt4914 as a code owner August 25, 2022 22:15
@JishnuGoyal
Copy link
Contributor Author

Please take an initial pass at this
PTAL @BenHenning

BenHenning
BenHenning reviewed Aug 26, 2022
View reviewed changes
app/src/main/java/org/oppia/android/app/topic/lessons/TopicLessonsFragmentPresenter.kt Outdated Show resolved Hide resolved
app/src/main/java/org/oppia/android/app/topic/lessons/TopicLessonsFragmentPresenter.kt Outdated Show resolved Hide resolved
app/src/main/res/drawable/border_white.xml Outdated Show resolved Hide resolved
app/src/main/res/drawable/divider_item_decoration_grey.xml Outdated Show resolved Hide resolved
app/src/main/res/drawable/divider_item_decoration_grey.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/lessons_completed_chapter_view.xml Show resolved Hide resolved
app/src/main/res/layout/lessons_completed_chapter_view.xml Show resolved Hide resolved
app/src/main/res/layout/lessons_completed_chapter_view.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/topic_lessons_story_summary.xml Show resolved Hide resolved
app/src/sharedTest/java/org/oppia/android/app/topic/lessons/TopicLessonsFragmentTest.kt Show resolved Hide resolved
BenHenning
BenHenning reviewed Aug 26, 2022
View reviewed changes
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @JishnuGoyal! Took a first pass. I think after these comments are addressed you can send the PR to Rajat for codeowners review.

@BenHenning BenHenning assigned JishnuGoyal and unassigned BenHenning Aug 26, 2022
@JishnuGoyal
Copy link
Contributor Author

Thanks @BenHenning
Please take another pass on this PR as I think there are a number of things that require another check.
@rt4914 would it be possible for you to PTAL for codeowners?

@JishnuGoyal JishnuGoyal assigned rt4914 and BenHenning and unassigned JishnuGoyal Aug 26, 2022
BenHenning
BenHenning reviewed Sep 11, 2022
View reviewed changes
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @JishnuGoyal. Took another pass. I think one of the past comments hasn't been addressed yet--please make sure to before sending this back.

domain/src/main/java/org/oppia/android/domain/topic/TopicListController.kt Show resolved Hide resolved
model/src/main/proto/topic.proto Outdated Show resolved Hide resolved
model/src/main/proto/topic.proto Show resolved Hide resolved
@BenHenning BenHenning assigned BenHenning and unassigned BenHenning Sep 11, 2022
rt4914
rt4914 suggested changes Sep 14, 2022
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these things:

  1. The Chapter name in RTL is left aligned but instead it should be right aligned.
  2. Make each chapter item minimum 48dp height similar to mocks https://xd.adobe.com/view/3dca36c2-5115-419c-b25e-0f10526b077c-6899/screen/558af437-bec4-4719-8d30-291da10bbc9d/specs/
  3. Add a video showcasing talkback-output correctly. Either it should have a video or captions on screen. https://developer.android.com/guide/topics/ui/accessibility/testing#optional_talkback_developer_settings && https://github.com/oppia/oppia-android/wiki/Accessibility-(A11y)-Guide

app/src/main/res/drawable/chapter_white_bg_with_bright_green_border.xml Show resolved Hide resolved
app/src/main/res/layout-sw600dp-land/topic_lessons_story_summary.xml Outdated Show resolved Hide resolved
@rt4914 rt4914 removed their assignment Sep 14, 2022
@JishnuGoyal JishnuGoyal mentioned this pull request Sep 17, 2022
Closed
@JishnuGoyal
Copy link
Contributor Author

PTAL @rt4914

@oppiabot oppiabot bot assigned rt4914 and unassigned JishnuGoyal Sep 17, 2022
@oppiabot
Copy link

oppiabot bot commented Sep 17, 2022

Unassigning @JishnuGoyal since a re-review was requested. @JishnuGoyal, please make sure you have addressed all review comments. Thanks!

rt4914
rt4914 approved these changes Sep 19, 2022
View reviewed changes
Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thanks.

@rt4914 rt4914 removed their assignment Sep 19, 2022
@JishnuGoyal
Copy link
Contributor Author

PTAL @BenHenning

BenHenning
BenHenning approved these changes Sep 20, 2022
View reviewed changes
Copy link
Member

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @JishnuGoyal! This LGTM!

@BenHenning BenHenning merged commit e91de5d into oppia:develop Sep 20, 2022
BenHenning added a commit that referenced this pull request Sep 20, 2022
@BenHenning
Revert "Fix #4449: Modify the design of the chapter list in lessons t…
3a98829 
…ab (#4535)"

This reverts commit e91de5d.
@BenHenning BenHenning mentioned this pull request Sep 20, 2022
Merged
BenHenning added a commit that referenced this pull request Sep 21, 2022
@BenHenning
Revert "Fix #4449: Modify the design of the chapter list in lessons t…
7e4f2dd 
…ab (#4535)" (#4603)

This reverts commit e91de5d.
@BenHenning BenHenning mentioned this pull request Sep 21, 2022
Closed
This was referenced Sep 22, 2022
Closed
Merged
BenHenning pushed a commit that referenced this pull request Sep 24, 2022
@JishnuGoyal
Fix #4602, #4449: Modify the design of the chapter list in lessons tab (
dc23e47 
#4621)

Includes the fast-forward fix so that develop doesn't break.

Original PR: #4535.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenHenning BenHenning BenHenning approved these changes

@rt4914 rt4914 rt4914 approved these changes

Assignees

@BenHenning BenHenning

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JishnuGoyal @rt4914 @BenHenning