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 #4648: Fix topic lesson differentiation between locked, completed and in-progress lessons #4958

Conversation

kkmurerwa
Copy link
Collaborator

Explanation

Fix #4648: When this PR is merged, it will fix the issue of locked, completed and in progress lessons not being differentiated by Talkback.

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

…ot differentiating between locked, completed and in-progress lessons
Copy link
Sponsor 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 @kkmurerwa! This seems like a really sensible solution. Had a few follow-up suggestions--PTAL.

@BenHenning BenHenning assigned kkmurerwa and unassigned BenHenning Apr 27, 2023
- Add a string for chapter locked state with no previous chapter title
- Add a default state for chapter locked with no previous chapter title
- Add a test for the new behaviour
@kkmurerwa kkmurerwa assigned BenHenning and unassigned kkmurerwa Apr 28, 2023
@kkmurerwa
Copy link
Collaborator Author

Screenshot(s) of the failing test when the new code block is commented out;
Screenshot from 2023-04-28 16-22-01
The log produced by the failure
Screenshot from 2023-04-28 16-22-45

Screenshot of the passing test with the new code block uncommented;
Screenshot from 2023-04-28 16-25-10

@kkmurerwa
Copy link
Collaborator Author

Hey @BenHenning. I have addressed your comments and added screenshots of the failing and passing test. You should be able to do another review. PTAL.

@kkmurerwa kkmurerwa assigned kkmurerwa and unassigned BenHenning May 2, 2023
@kkmurerwa
Copy link
Collaborator Author

Hey @BenHenning. I have temporarily reassigned myself the issue. There are a few more changes I am working on to improve accessibility.

@kkmurerwa
Copy link
Collaborator Author

Hey @BenHenning. Here are a few changes I have made with commit ID 8d497a2.

  • I noticed that there are no existing tests to check for the computeChapterPlayStateIconContentDescription that is displayed on the chapter_play_state_icon. I added a test for this that tests in the content description for progress chapters is being displayed correctly.
  • In the case of a completed chapter topic, there is no chapter play state icon displayed. I modified the chapter_index text view to display the string from computeChapterPlayStateIconContentDescription instead for better Talkback feedback to visually impaired users.
  • I added a test for the computeChapterPlayStateIconContentDescription in the case of the completed chapter topic state.

Here is a screenshot of the failing tests when I return a dummy string.
Screenshot from 2023-05-02 18-31-56

Here is a screenshot of the passing tests when the original logic is reinstated.
Screenshot from 2023-05-02 18-35-12

The tests on the CI failed for some reason but they all pass locallly. The failing test requires a rerun to pass.
Screenshot from 2023-05-02 18-45-16

@kkmurerwa kkmurerwa assigned BenHenning and unassigned kkmurerwa May 2, 2023
@BenHenning
Copy link
Sponsor Member

Apologies @kkmurerwa will need to look at this tomorrow.

@kkmurerwa
Copy link
Collaborator Author

No problem at all Ben.

Copy link
Sponsor 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 @kkmurerwa! PR LGTM, just had small nits--PTAL. Also, the branch will need to be brought up-to-date with the latest develop so that we can merge it.

@BenHenning BenHenning assigned kkmurerwa and unassigned BenHenning May 4, 2023
@kkmurerwa
Copy link
Collaborator Author

Hey @BenHenning. The issues mentioned have been resolved. The PR should now be ready for a merge unless there are some other issues.

@kkmurerwa kkmurerwa removed their assignment May 4, 2023
Copy link
Sponsor 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 @kkmurerwa! Apologies for the delayed review. This LGTM!

@BenHenning BenHenning merged commit c5eac6d into oppia:develop May 9, 2023
36 checks passed
@kkmurerwa kkmurerwa deleted the differentiate-locked-completed-and-in-progress-lessons branch November 1, 2023 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No differentiation between completed lessons and locked lessons in topics tab
2 participants