-
Notifications
You must be signed in to change notification settings - Fork 529
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 #4397, #4923: Change the closing button description for consistency & gender neutral profile image #4953
Conversation
Hi @XichengSpencer. Thanks for your PR. Can you please add a recording in the description that shows the fix is working? Also counter-check the essential checklist and ensure your PR meets all that are applicable. |
Here is the recording https://user-images.githubusercontent.com/74568012/233457088-8d26b9e0-c05b-485f-8e6c-1252f76871cd.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks correct.
@BenHenning, do we generally add automated tests that verify content descriptions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @XichengSpencer!
This looks correct. @BenHenning, do we generally add automated tests that verify content dsecriptions?
@adhiamboperes @XichengSpencer Yep, we should add a test for situations that can be tested (and content descriptions generally can be). Please do add a test to verify this change @XichengSpencer.
app/src/main/java/org/oppia/android/app/topic/conceptcard/ConceptCardFragmentPresenter.kt
Show resolved
Hide resolved
app/src/main/java/org/oppia/android/app/topic/conceptcard/ConceptCardFragmentPresenter.kt
Show resolved
Hide resolved
For #4923 here are the result screenshots |
Hey @XichengSpencer. I would advise that you open one PR per issue that you are working on, for now. |
Sorry, I forgot to create a new branch for this one. I will definitely keep it in mind for the next time. But what should I do for this one? The second issue is just a minor resource change here. |
We can keep this one this time, although in future only close multiple issues in one PR if they are thematically related. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @XichengSpencer. Your changes look good to me.
Can you also fill out the screenshots section of the PR description and then assign your pr over to Ben by tagging him and mentioning "PTAL"?
Done. Thank you so much for the review! |
Hi @BenHenning |
Unassigning @XichengSpencer since a re-review was requested. @XichengSpencer, please make sure you have addressed all review comments. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @XichengSpencer! This LGTM from a code perspective.
@seanlip could you or someone from UX weigh in on the icon change before we finalize this & merge?
Hm, I'm a bit dubious about the new icon. I'm a bit unsure about using the outline version instead of the filled version. Let me ask the design team for some feedback -- stay tuned. I'll send an email. |
Assigning to @kaiyuxu (Android design lead) for feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @XichengSpencer. Waiting on the design team's feedback before approving in case there are any additional changes needed.
app/src/main/java/org/oppia/android/app/topic/conceptcard/ConceptCardFragmentPresenter.kt
Show resolved
Hide resolved
@BenHenning Confirming this is all fine from a design and product perspective (see email) -- let's get this in. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @seanlip for confirming, and thanks @XichengSpencer for wrapping this up! LGTM.
Updating the PR to the latest develop & enabling auto-merge. |
Unassigning @BenHenning since they have already approved the PR. |
Hi @XichengSpencer, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to ask someone to merge your PR once the CI checks pass and you're happy with it. Thanks! |
Fixes #4397: The closing button on the nav bar of concept card activity had inconsistent voice output, it is changed to "navigate up" for consistency.
Fixes #4923: "Have outline person as default gender-neutral image"
Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then: