-
Notifications
You must be signed in to change notification settings - Fork 517
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 #431: Fixed Profile Page Cut-Off For Small Display #3630
Fix #431: Fixed Profile Page Cut-Off For Small Display #3630
Conversation
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 for the PR @Pranav-Bobde .
Three suggestions:
- There should be End-of-file on each file. Basically one blank line after all the code in the file.
- All dimens should be in multiple of 4.
- Add screenshots of this screen for mobile portrait, mobile landscape, tablet portrait and tablet landscape.
working on it |
@Pranav-Bobde Please comment on the issue not on this pull-request so that I can assign it to you. #431 |
|
Unassigning @Pranav-Bobde since a re-review was requested. @Pranav-Bobde, 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.
Also, @Pranav-Bobde Please do not resolve comments on your own. Just reply to the comments and the person who has started the comment thread will verify it based upon your answer and then resolve it.
Also in first screenshot the space above and below the line looks inconsistent can you please fix that too. |
…fragment-fix-profile-image-cut-off
…ttps:https://github.com/Pranav-Bobde/oppia-android into profile-chooser-fragment-fix-profile-image-cut-off
@rt4914 Yeah, it's the same gap that i asked in my doc. I did try to look in the xml files you mentioned too but i couldn't find it. So, if you could help find it will try to alter it accordingly ASAP. 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.
@Pranav-Bobde Checkout profile_chooser_add_view.xml
in this you might need to create a dimens value of profile_chooser_add_view_margin_top_profile_not_added
on smaller devices.
Also, can you add screenshots of before/after images of multiple
profile for all these screens so that we can be sure that when there are multiple profiles the UI is not affected because of this PR.
…fragment-fix-profile-image-cut-off
…ttps:https://github.com/Pranav-Bobde/oppia-android into profile-chooser-fragment-fix-profile-image-cut-off
@rt4914 Thanks. |
…fragment-fix-profile-image-cut-off
…ttps:https://github.com/Pranav-Bobde/oppia-android into profile-chooser-fragment-fix-profile-image-cut-off
@rt4914 |
@Pranav-Bobde I think you cannot re-run the checks right now. I have re-run the checks. Also, you can do two things:
|
@rt4914 Actually i had already updated the screenshots in the ppt as per your comments. And replied there to review them. So, if they all look good i will update them here too. |
@rt4914 Have updated the screenshots and added the link to PPT too. PTAL. |
Unassigning @Pranav-Bobde since a re-review was requested. @Pranav-Bobde, 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.
@Pranav-Bobde Overall looks really good. Just some final nit changes and it will be good to merge.
Unassigning @rt4914 since the review is done. |
Hi @Pranav-Bobde, it looks like some changes were requested on this pull request by @rt4914. PTAL. Thanks! |
…ttps:https://github.com/Pranav-Bobde/oppia-android into profile-chooser-fragment-fix-profile-image-cut-off
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, thanks.
Explanation
Fixes #431: Added Proper dimensions for profile_chooser_fragment.xml and profile_chooser_profile_view.xml
Screenshots:
Link For PPT : https://docs.google.com/presentation/d/1xDsWh7fNsDh4Ux8AJNPUIPyaSqSHbBan/edit#slide=id.p3
BEFORE AFTER
PORTRAIT - 3.4 WQVGA (ldpi)
LANDSCAPE - 3.4 WQVGA (ldpi)
PORTRAIT - 5.4 FWVGA (mdpi)
LANDSCAPE - 5.4 FWVGA (mdpi)
PORTRAIT - Nexus S (hdpi)
LANDSCAPE - Nexus S (hdpi)
PORTRAIT - Pixel (420dpi)
LANDSCAPE - Pixel (420dpi)
PORTRAIT - 10.1 WXGA (Tablet) (mdpi)
LANDSCAPE - 10.1 WXGA (Tablet) (mdpi)