Fix #4450: Add next and previous card options on revision screen #4554

JishnuGoyal · 2022-09-05T17:34:16Z

Explanation

This PR creates navigation cards on the revision card screen so that the users can easily view the next revision item without having to return to the topic screen, thus fixing the issue #4450 as a part of the Interactive Onboarding Flow (GSoC) project. The current implementation makes sure that the functionality can be viewed correctly on a mobile screen.
This PR is backed up by unit tests.

Link to the original mock: https://xd.adobe.com/view/3dca36c2-5115-419c-b25e-0f10526b077c-6899/screen/9c633b9e-aae5-428d-a037-efcc0338c4df/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

Mobile (Portrait)

When next and prev both are available
When only next is available
When only previous is available

Mobile (Landscape)

Tablet (Portrait) - prev button only

Tablet (Portrait) - both buttons

Tablet (portrait) - next button only

Tablet (Landscape)

RTL:

RTL (land):

Confirming that the Espresso tests are passing locally:

Functionality demo:
https://user-images.githubusercontent.com/64526117/192758757-37e18649-2a5f-48dd-8917-19d5c15b143d.mp4

TalkBack:https://user-images.githubusercontent.com/64526117/192503526-8ef9100d-083e-40ef-ad17-04f857955354.mp4

# Conflicts: # app/src/main/res/values/strings.xml

JishnuGoyal · 2022-09-05T17:41:18Z

PTAL @BenHenning please take an initial pass -- does the general implementation look correct?

BenHenning · 2022-09-06T07:57:58Z

PTAL @BenHenning please take an initial pass -- does the general implementation look correct?

Hi @JishnuGoyal. I didn't review it in detail (i.e. I have a bunch of more detailed feedback that I can give), but I think it looks solid at a high-level. I do have some high-level feedback:

It looks like you had extra strings pulled in--please make sure to fix this.
I suggest controlling the visibility of the tiles via databinding instead of directly in Kotlin. It's a bit more idiomatic for how we usually do things, and I suspect it may simplify things in the binding flow a bit.
Consider combining the next/previous state into a single LiveData with an object that can represent the four different possible states: no previous/next, one previous no next, no previous one next, one previous one next. A sealed object would be a really nice fit here, but I don't think that will work in databinding so maybe just a simple, single data class or perhaps a proto oneof would work best.
Before sending this for review, I also suggest: adding missing tests, fixing CI checks, adding tablet/portrait screenshots to the PR description, demonstrate the a11y flow via a video in the PR description, and adding RTL screenshots to the PR description.

JishnuGoyal · 2022-09-10T01:09:57Z

PTAL @BenHenning , thanks!

oppiabot · 2022-09-10T01:13:00Z

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

BenHenning

Thanks @JishnuGoyal! Some top-level thoughts:

Could you include two brief videos in the PR description showing you playing through and clicking the next/previous buttons with and without TalkBack (i.e. one video for TalkBack, one for not)? I think seeing how this will actually behave will help better contextualize your screenshots.
Why are the colors different between the tablet & non-tablet screenshots? Something seems wrong there.
For RTL, it seems like the header text isn't being reversed to the other side of the layout--is the wrong style being used for that TextView?
Please include screenshots for RevisionCardFragmentTest passing on Espresso since it's been changed.
It would be nice, I think, to include at least one screenshot of the new UI with dark mode just to see how it looks. While we're not factoring in dark mode fully for each individual PR, there are some color changes here that I'm wondering about.
The header looks a bit weird to me both for the "next button only" case, and on tablet in general. For the latter, it probably needs to be bigger. For the former, maybe a bigger header would help but I'm actually wondering if perhaps the button needs to be centered. I suggest emailing @mschanteltc and showing her the screenshots from this PR to get her thoughts on what we might be able to tweak to help with how it looks.
CI checks are failing--PTAL.

app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardActivity.kt

app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardFragmentPresenter.kt

app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardViewModel.kt

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

app/src/sharedTest/java/org/oppia/android/app/topic/revisioncard/RevisionCardFragmentTest.kt

domain/src/main/java/org/oppia/android/domain/topic/TopicListController.kt

mschanteltc · 2022-09-15T19:09:57Z

Thank you Jishnu. I agree with Ben that when "only next is available," the current formatting looks a bit weird. Here are my suggestions for improvement:

All Layouts
Create a gray background (#F2F2F2) for the "Continue Studying" section.

Mobile (Portrait)
"Next only": Right-align the "Continue Studying" text and right-align its textbox with respect to its container. Right-align the Subtopic card (which is already done).

Mobile (Landscape) and Tablet (Portrait, Landscape)
Container width: Let's make the max container width that holds the heading and cards 480px. That way, elements in this section look more associated with each other. (Please correct me if I am using the correct terminology). I added links to the proposed designs and an image below. Let me know what you think!

Mobile, Portrait [Prev and Next] [Next Only]
Mobile, Landscape [Prev and Next] [Next Only]
Tablet, Portrait [Prev and Next] [Next Only]
Tablet, Landscape [Prev and Next] [Next Only]

oppiabot · 2022-09-22T19:13:40Z

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

BenHenning

Thanks @JishnuGoyal. Mostly LGTM. Just had a few nits left. Please also wait for CI to finish & fix all failures before sending back.

app/src/sharedTest/java/org/oppia/android/app/topic/revisioncard/RevisionCardFragmentTest.kt

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

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

app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardViewModel.kt

app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardFragmentPresenter.kt

app/src/main/java/org/oppia/android/app/topic/revision/TopicRevisionFragmentPresenter.kt

app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardActivity.kt

# Conflicts: # app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardActivity.kt # app/src/main/res/values/component_colors.xml # app/src/sharedTest/java/org/oppia/android/app/topic/revisioncard/RevisionCardFragmentTest.kt

JishnuGoyal · 2022-10-01T13:27:32Z

PTAL @BenHenning

oppiabot · 2022-10-01T13:30:35Z

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

BenHenning

Thanks @JishnuGoyal. Just a few small things left (I think one of my past KDoc comments was a bit unclear, but I've clarified it now).

app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardViewModel.kt

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

app/src/sharedTest/java/org/oppia/android/app/topic/revisioncard/RevisionCardActivityTest.kt

JishnuGoyal · 2022-10-02T19:08:24Z

PTAL @BenHenning

oppiabot · 2022-10-02T19:11:27Z

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

BenHenning

@JishnuGoyal I can't approve this PR yet because one of the comments wasn't fully addressed. PTAL.

app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardViewModel.kt

BenHenning

Thanks @JishnuGoyal! PR LGTM.

oppiabot · 2022-10-02T22:33:44Z

Assigning @rt4914 for code owner reviews. Thanks!

add functionality for next/prev navigation for revision cards

05e0966

JishnuGoyal requested review from rt4914 and BenHenning as code owners September 5, 2022 17:34

Merge branch 'develop' into 1.6_revision_screen

d5b626b

# Conflicts: # app/src/main/res/values/strings.xml

oppiabot bot assigned BenHenning Sep 5, 2022

BenHenning assigned JishnuGoyal and unassigned BenHenning Sep 6, 2022

JishnuGoyal added 5 commits September 9, 2022 17:38

Merge branch 'develop' into 1.6_revision_screen

1472d69

update according to changes from develop

5e430f5

fix bad merge with develop

23f44b5

make toolbar orange

01c56f2

refactor code + add tests

6a3cabc

lint/shorten test name

7c51cac

oppiabot bot assigned BenHenning and unassigned JishnuGoyal Sep 10, 2022

lint/shorten test name

b3d75a4

BenHenning requested changes Sep 10, 2022

View reviewed changes

BenHenning assigned JishnuGoyal and unassigned BenHenning Sep 10, 2022

JishnuGoyal added 4 commits September 11, 2022 19:50

add all refactor jobs

3d4c9f2

requested changes

e597b46

add 'scroll to' to tests

749576b

add min height and width

5d94adf

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

JishnuGoyal assigned BenHenning and unassigned JishnuGoyal Sep 30, 2022

BenHenning reviewed Sep 30, 2022

View reviewed changes

BenHenning assigned JishnuGoyal and unassigned BenHenning Sep 30, 2022

JishnuGoyal added 5 commits October 1, 2022 12:06

fix todo spacing

994ddb5

wrap up with review comments and fix CI

3c0e64d

lint

b122dce

text alignment should be right when only next card is visible

9f8d9ff

oppiabot bot assigned BenHenning and unassigned JishnuGoyal Oct 1, 2022

BenHenning reviewed Oct 1, 2022

View reviewed changes

BenHenning assigned JishnuGoyal and unassigned BenHenning Oct 1, 2022

add kdocs

8eeecb7

oppiabot bot assigned BenHenning and unassigned JishnuGoyal Oct 2, 2022

BenHenning reviewed Oct 2, 2022

View reviewed changes

app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardViewModel.kt Show resolved Hide resolved

BenHenning assigned JishnuGoyal and unassigned BenHenning Oct 2, 2022

finish up kdocs

66e10b6

BenHenning approved these changes Oct 2, 2022

View reviewed changes

oppiabot bot assigned rt4914 Oct 2, 2022

BenHenning merged commit f096b1d into oppia:develop Oct 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #4450: Add next and previous card options on revision screen #4554

Fix #4450: Add next and previous card options on revision screen #4554

JishnuGoyal commented Sep 5, 2022 •

edited

Loading

JishnuGoyal commented Sep 5, 2022

BenHenning commented Sep 6, 2022 •

edited

Loading

JishnuGoyal commented Sep 10, 2022

oppiabot bot commented Sep 10, 2022

BenHenning left a comment

mschanteltc commented Sep 15, 2022 •

edited

Loading

oppiabot bot commented Sep 22, 2022

BenHenning left a comment

JishnuGoyal commented Oct 1, 2022

oppiabot bot commented Oct 1, 2022

BenHenning left a comment

JishnuGoyal commented Oct 2, 2022

oppiabot bot commented Oct 2, 2022

BenHenning left a comment

BenHenning left a comment

oppiabot bot commented Oct 2, 2022

Fix #4450: Add next and previous card options on revision screen #4554

Fix #4450: Add next and previous card options on revision screen #4554

Conversation

JishnuGoyal commented Sep 5, 2022 • edited Loading

Explanation

Essential Checklist

For UI-specific PRs only

JishnuGoyal commented Sep 5, 2022

BenHenning commented Sep 6, 2022 • edited Loading

JishnuGoyal commented Sep 10, 2022

oppiabot bot commented Sep 10, 2022

BenHenning left a comment

Choose a reason for hiding this comment

mschanteltc commented Sep 15, 2022 • edited Loading

oppiabot bot commented Sep 22, 2022

BenHenning left a comment

Choose a reason for hiding this comment

JishnuGoyal commented Oct 1, 2022

oppiabot bot commented Oct 1, 2022

BenHenning left a comment

Choose a reason for hiding this comment

JishnuGoyal commented Oct 2, 2022

oppiabot bot commented Oct 2, 2022

BenHenning left a comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

oppiabot bot commented Oct 2, 2022

JishnuGoyal commented Sep 5, 2022 •

edited

Loading

BenHenning commented Sep 6, 2022 •

edited

Loading

mschanteltc commented Sep 15, 2022 •

edited

Loading