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.

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

Conversation

Pranav-Bobde
Copy link
Contributor

@Pranav-Bobde Pranav-Bobde commented Aug 1, 2021

Explanation

Fixes #431: Added Proper dimensions for profile_chooser_fragment.xml and profile_chooser_profile_view.xml

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The PR follows the style guide.
  • The PR does not contain any unnecessary auto-generated code from Android Studio.
  • The PR is made from a branch that's not called "develop".
  • The PR is made from a branch that is up-to-date with "develop".
  • The PR's branch is based on "develop" and not on any other branch.
  • The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

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)
            

@rt4914 rt4914 self-assigned this Aug 2, 2021
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.

Thanks for the PR @Pranav-Bobde .
Three suggestions:

  1. There should be End-of-file on each file. Basically one blank line after all the code in the file.
  2. All dimens should be in multiple of 4.
  3. Add screenshots of this screen for mobile portrait, mobile landscape, tablet portrait and tablet landscape.

app/src/main/res/values-hdpi/dimens.xml Outdated Show resolved Hide resolved
app/src/main/res/values-hdpi/dimens.xml Outdated Show resolved Hide resolved
app/src/main/res/values-hdpi/dimens.xml Outdated Show resolved Hide resolved
app/src/main/res/values-ldpi/dimens.xml Outdated Show resolved Hide resolved
app/src/main/res/values-mdpi/dimens.xml Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned Pranav-Bobde and unassigned rt4914 Aug 2, 2021
@Pranav-Bobde
Copy link
Contributor Author

working on it

@rt4914
Copy link
Contributor

rt4914 commented Aug 4, 2021

working on it

@Pranav-Bobde Please comment on the issue not on this pull-request so that I can assign it to you. #431

@Pranav-Bobde
Copy link
Contributor Author

working on it

@Pranav-Bobde Please comment on the issue not on this pull-request so that I can assign it to you. #431
@rt4914 Sorry about that.
Added the Screenshots, PTAL.

@oppiabot oppiabot bot assigned rt4914 and unassigned Pranav-Bobde Aug 4, 2021
@oppiabot
Copy link

oppiabot bot commented Aug 4, 2021

Unassigning @Pranav-Bobde since a re-review was requested. @Pranav-Bobde, please make sure you have addressed all review comments. Thanks!

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.

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.

app/src/main/res/values/dimens.xml Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned Pranav-Bobde and unassigned rt4914 Aug 6, 2021
@rt4914
Copy link
Contributor

rt4914 commented Aug 6, 2021

Also in first screenshot the space above and below the line looks inconsistent can you please fix that too.

@Pranav-Bobde
Copy link
Contributor Author

Pranav-Bobde commented Aug 6, 2021

Also in first screenshot the space above and below the line looks inconsistent can you please fix that too.

@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.

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.

@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.

@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Aug 30, 2021
@Pranav-Bobde
Copy link
Contributor Author

Pranav-Bobde commented Aug 30, 2021

@rt4914
Have updated the PPT with new Screenshots. Please let me know if they all seem right.

Thanks.

@Pranav-Bobde
Copy link
Contributor Author

@rt4914
2 checks seems to be failing. Can you tell me how to redo the check? since i guess in the past too just renunning the checks used to resolve the problem.

@rt4914 rt4914 assigned rt4914 and unassigned Pranav-Bobde Sep 1, 2021
@rt4914
Copy link
Contributor

rt4914 commented Sep 6, 2021

@Pranav-Bobde I think you cannot re-run the checks right now. I have re-run the checks. Also, you can do two things:

  1. Update with develop
  2. Also can you update screenshots either here or in ppt so that we can see if screenshots are looking good.

@rt4914 rt4914 assigned Pranav-Bobde and unassigned rt4914 Sep 6, 2021
@Pranav-Bobde
Copy link
Contributor Author

@Pranav-Bobde I think you cannot re-run the checks right now. I have re-run the checks. Also, you can do two things:

  1. Update with develop
  2. Also can you update screenshots either here or in ppt so that we can see if screenshots are looking good.

@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.

@Pranav-Bobde
Copy link
Contributor Author

@rt4914 Have updated the screenshots and added the link to PPT too. PTAL.

@oppiabot oppiabot bot assigned rt4914 and unassigned Pranav-Bobde Sep 8, 2021
@oppiabot
Copy link

oppiabot bot commented Sep 8, 2021

Unassigning @Pranav-Bobde since a re-review was requested. @Pranav-Bobde, please make sure you have addressed all review comments. Thanks!

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.

@Pranav-Bobde Overall looks really good. Just some final nit changes and it will be good to merge.

app/src/main/res/values-xxhdpi/dimens.xml Outdated Show resolved Hide resolved
app/src/main/res/values-mdpi/dimens.xml Outdated Show resolved Hide resolved
app/src/main/res/values-ldpi/dimens.xml Outdated Show resolved Hide resolved
app/src/main/res/values-land-ldpi/dimens.xml Show resolved Hide resolved
app/src/main/res/values-land-mdpi/dimens.xml Outdated Show resolved Hide resolved
@oppiabot oppiabot bot unassigned rt4914 Sep 9, 2021
@oppiabot
Copy link

oppiabot bot commented Sep 9, 2021

Unassigning @rt4914 since the review is done.

@oppiabot
Copy link

oppiabot bot commented Sep 9, 2021

Hi @Pranav-Bobde, it looks like some changes were requested on this pull request by @rt4914. PTAL. Thanks!

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.

LGTM, thanks.

@rt4914 rt4914 merged commit f56345c into oppia:develop Sep 10, 2021
@Pranav-Bobde Pranav-Bobde deleted the profile-chooser-fragment-fix-profile-image-cut-off branch September 11, 2021 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Profile page is cut off with small display size
3 participants