-
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 #3453 & #3454: Optimise UI pin related screens - pin_password_activity, admin_auth_activity, admin_pin_activity #4355
Conversation
Thanks for submitting this pull request! Some main reviewers |
Hi @rt4914, it looks like some changes were requested on this pull request by @vrajdesai78. PTAL. 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.
Approving per @rt4914's request since this needs a reviewer with write access to do so.
/cc @BenHenning FYI.
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.
LGTM @rt4914 just left a comment PTAL thanks
@@ -3,6 +3,7 @@ | |||
<!-- Default screen margins, per the Android Design guidelines. --> | |||
<dimen name="activity_horizontal_margin">16dp</dimen> | |||
<dimen name="activity_vertical_margin">16dp</dimen> | |||
<dimen name="match_parent">-2px</dimen> |
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.
why have you used -ive margin for match_parent ?
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.
Firstly, this is not the best industry practice but for our case it is the best solution.
Because we want to merge all files into one file but the issue is a view can be match_parent
or 360dp
(something fixed). Now in dimens.xml file we cannot mention match_parent
directly but the equivalent of that is -2px
and similarly -1px
is wrap_content
. This -2px
is actually not a value that will get used instead it denotes that the OS has to use match_parent
here.
Explanation
Fix #3453 & Fix #3454: Optimise UI pin related screens -
pin_password_activity
,admin_auth_activity
,admin_pin_activity
All these mocks can be found from links given here: https://github.com/oppia/oppia-android/wiki/Working-on-UI
This is basically complete redesign of all three screens because the earlier implementations either had too-many files or were not optimised.
You can follow this video link to understand why and how I applied the designs https://youtu.be/fLP4bm3D6Cw
Essential Checklist
Pin Password Activity
Admin Auth Activity
Admin Pin Activity
RTL Screenshots
Note: There are some issues in screenshot but I have checked on develop and the results are same. There can be two issues here (a.) the emulator itself is unable to give correct results (b.) for stings which contain digit the translation is not working. In either case it will be investigated and solved as a separate issue.