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

[JishnuGoyal]

Explanation

Fixes #4450

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

• [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 files have their rationale included in the PR explanation.
• [x] The PR follows the style guide.
• [x] The PR does not contain any unnecessary code changes from Android Studio (reference).
• [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).

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)

1. When next and prev both are available

2. When only next is available

3. 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):

[JishnuGoyal]

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

[BenHenning]

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]

PTAL @BenHenning , thanks!

[oppiabot[bot]]

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

[mschanteltc]

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[bot]]

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!

[JishnuGoyal]

Few parts that are left:

1. Respond to Chantel's suggestions
2. Update the screenshots (mobile + tablet) with the latest screenshots
3. Add a video playing through the functionality introduced
4. Add todos in the code where the disableAccessibilityChecks annotation has been added
5. One test probably needs to be added to check if the header aligns itself to the right when only next revision card is available.

PTAL @BenHenning, thanks.

[oppiabot[bot]]

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

[JishnuGoyal]

Thank you so much @mschanteltc for your valuable suggestions. I find the new design ideas really nice!

@BenHenning I think the technical problem before we implement @mschanteltc's suggestions is that in the tablet mode, the width of the screen keeps changing depending upon the amount of content that is shown. The problem with navigation cards placement itself then boils down to the issue that a constant width for tablets isn't set. I think this problem deserves a separate issue.

For this PR, i plan to fix the header alignment as suggested by Chantel, by aligning it to the right when only the next navigation card is visible.

Please let me know what are your thoughts @BenHenning @mschanteltc.

Thanks

[JishnuGoyal]

PTAL @BenHenning

[oppiabot[bot]]

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

[JishnuGoyal]

PTAL @BenHenning

[oppiabot[bot]]

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

[oppiabot[bot]]

Assigning @rt4914 for code owner reviews. Thanks!