-
Notifications
You must be signed in to change notification settings - Fork 499
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 #3495 and #3496: Add support for displaying dependencies list and their license texts #3501
Conversation
c6ed7d9
to
8869156
Compare
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.
@prayutsu I have checked the PR from ViewModel's perspective. It does look good. I have left some other comments which I thought of.
Also, note that once the PR is complete from your side, you will need to add RTL
and A11y
output too.
app/src/main/java/org/oppia/android/app/help/thirdparty/ThirdPartyDependencyListActivity.kt
Outdated
Show resolved
Hide resolved
...main/java/org/oppia/android/app/help/thirdparty/ThirdPartyDependencyListActivityPresenter.kt
Outdated
Show resolved
Hide resolved
...main/java/org/oppia/android/app/help/thirdparty/ThirdPartyDependencyListFragmentPresenter.kt
Outdated
Show resolved
Hide resolved
...main/java/org/oppia/android/app/help/thirdparty/ThirdPartyDependencyListFragmentPresenter.kt
Outdated
Show resolved
Hide resolved
...main/java/org/oppia/android/app/help/thirdparty/ThirdPartyDependencyListFragmentPresenter.kt
Outdated
Show resolved
Hide resolved
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 @prayutsu. Mostly nits left. PTAL.
app/src/main/java/org/oppia/android/app/help/thirdparty/RouteToLicenseTextListener.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/android/app/help/thirdparty/RouteToLicenseListListener.kt
Outdated
Show resolved
Hide resolved
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 @prayutsu. Primarily one follow-up for something that I missed earlier. PTAL.
@BenHenning PTAL. |
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 @prayutsu. Per codeowners, this LGTM, just had 1 nit. PTAL.
Also, @prayutsu could you please update this to the latest develop & address the conflict? |
@BenHenning PTAL. |
Hi. Will need to look at this tomorrow. |
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 @prayutsu. LGTM!
Explanation
Fixes #3495 and Fixes #3496:
Hooked up a new UI to display the third-party dependencies used to build Oppia Android.
Mock link - https://xd.adobe.com/view/e8aa4198-3940-47f9-514a-f41cc54457f6-9e9b/screen/0f94055a-c5f7-45a2-8bf5-9dcfdf1c7dce/specs/
a11y
a11y with RTL
Tests
Checklist