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 #4909 : Remove the unused condition from various lessons_chapters_view for Accessibility #4910

Merged
merged 34 commits into from
Jul 4, 2023
Merged

Fix #4909 : Remove the unused condition from various lessons_chapters_view for Accessibility #4910

merged 34 commits into from
Jul 4, 2023

Conversation

MohitGupta121
Copy link
Member

@MohitGupta121 MohitGupta121 commented Mar 18, 2023 •
edited
Loading

Explanation

Fixed #4909 : Remove the unused condition from lessons_no_started_chapter_view.xml lessons_in_progress_chapter_view.xml lessons_locked_chapter_view.xml for Accessibility

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

rt4914
rt4914 suggested changes Mar 18, 2023
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.

@MohitGupta121 PTAL

@rt4914 rt4914 assigned MohitGupta121 and unassigned rt4914 Mar 18, 2023
rt4914
rt4914 suggested changes Mar 18, 2023
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.

@MohitGupta121 Sorry, in previous PR I wrote those comments for incorrect file. Actually those changes should be applied for lessons_locked_chapter_view

app/src/main/res/layout/lessons_locked_chapter_view.xml Show resolved Hide resolved
@MohitGupta121 MohitGupta121 removed their assignment Mar 18, 2023
MohitGupta121
MohitGupta121 commented Mar 18, 2023
View reviewed changes
Copy link
Member Author

@MohitGupta121 MohitGupta121 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@rt4914 PTAL, updated the requested changes.
Thanks

@rt4914 rt4914 removed their assignment Mar 18, 2023
BenHenning
BenHenning previously requested changes Mar 22, 2023
View reviewed changes
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 @MohitGupta121! This is a nice simplification, though I had one question regarding the user experience since I think we might be able to make this a better experience with some adjustments. PTAL.

app/src/main/res/layout/lessons_in_progress_chapter_view.xml Outdated Show resolved Hide resolved
app/src/main/res/layout/lessons_locked_chapter_view.xml Show resolved Hide resolved
@rt4914 rt4914 assigned rt4914 and unassigned MohitGupta121 Mar 28, 2023
@oppiabot
Copy link

oppiabot bot commented Apr 4, 2023

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

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Apr 4, 2023
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Apr 4, 2023
MohitGupta121
MohitGupta121 commented Jun 15, 2023
View reviewed changes
Copy link
Member Author

@MohitGupta121 MohitGupta121 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@adhiamboperes PTAL, now I fix the duplicate talkback response, please see in video. Thanks!

Talkback.lock.chapter.webm

@kkmurerwa
Copy link
Collaborator

Hi @MohitGupta121. Sorry, I have taken this long to review your PR. Going by the most recent recording, the issue @adhiamboperes mentioned seems to have been fixed. Please fix the failing checks and update the PR with the latest code from the main branch and I will take a full pass at it.

@MohitGupta121
Copy link
Member Author

@kkmurerwa It's okay, no problem. Thanks, I will working on updating the tests.

@MohitGupta121 MohitGupta121 removed their assignment Jul 1, 2023
MohitGupta121
MohitGupta121 commented Jul 1, 2023
View reviewed changes
Copy link
Member Author

@MohitGupta121 MohitGupta121 left a comment

Choose a reason for hiding this comment

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

@adhiamboperes PTAL, now all Tests are passing, all fixed. Thanks!

adhiamboperes
adhiamboperes approved these changes Jul 3, 2023
View reviewed changes
Copy link
Collaborator

@adhiamboperes adhiamboperes left a comment

Choose a reason for hiding this comment

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

Hi @MohitGupta121, I had a minor comment, otherwise this PR LGTM!

app/src/main/res/values/strings.xml Outdated Show resolved Hide resolved
@adhiamboperes adhiamboperes removed the request for review from kkmurerwa July 3, 2023 13:44
@oppiabot oppiabot bot added the PR: LGTM label Jul 3, 2023
@oppiabot
Copy link

oppiabot bot commented Jul 3, 2023

Hi @MohitGupta121, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

MohitGupta121
MohitGupta121 commented Jul 3, 2023
View reviewed changes
Copy link
Member Author

@MohitGupta121 MohitGupta121 left a comment

Choose a reason for hiding this comment

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

@adhiamboperes PTAL, now string naming is corrected. Thanks!

@adhiamboperes adhiamboperes merged commit f93c06b into oppia:develop Jul 4, 2023
36 checks passed
@MohitGupta121 MohitGupta121 deleted the remove_unused_conditions branch July 4, 2023 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rt4914 rt4914 rt4914 approved these changes

@kkmurerwa kkmurerwa kkmurerwa left review comments

@adhiamboperes adhiamboperes adhiamboperes approved these changes

@BenHenning BenHenning BenHenning left review comments

Assignees

@adhiamboperes adhiamboperes

Labels
PR: LGTM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove the unused condition from various lessons_chapters_view for Accessibility
5 participants
@MohitGupta121 @kkmurerwa @rt4914 @BenHenning @adhiamboperes