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 #3359: Convert welcome.xml into single layout file pull request for issue #3368

Merged
merged 7 commits into from
Jun 29, 2021

Conversation

TheSwarnim
Copy link
Contributor

@TheSwarnim TheSwarnim commented Jun 23, 2021

Explanation

Fixes #3359
This PR Convert welcome.xml into a single layout file. In this PR welcome.xml files for different orientations such as land, sw600dp-land and sw600dp-port are deleted and all rendering of the screen is done by the main welcome.xml, and the margin & padding changes dynamically using dimens.xml accordingly.
Please note that to run the project one has to rebuild the project because deleting the orientations welcome.xml files some changes were taking place (Since I don't know this, I struggle for a long time) otherwise it will not work on tablet modes.

Checklist

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

@TheSwarnim TheSwarnim requested a review from rt4914 as a code owner June 23, 2021 13:38
@anandwana001
Copy link
Contributor

Hi @TheSwarnim
Congratulations on your first Pull Request.

Please follow the first two points from the checklist given in the description section.

@TheSwarnim TheSwarnim changed the title Convert welcome.xml into single layout file pull request for issue #3359 Fix #3359: Convert welcome.xml into single layout file pull request for issue Jun 23, 2021
@TheSwarnim
Copy link
Contributor Author

Hi @TheSwarnim
Congratulations on your first Pull Request.

Please follow the first two points from the checklist given in the description section.

Thank you, sir, I'll continue to give contributions to this organization, and wants to learn from you in the coming days

@rt4914
Copy link
Contributor

rt4914 commented Jun 24, 2021

Hi @TheSwarnim
Congratulations on your first Pull Request.
Please follow the first two points from the checklist given in the description section.

Thank you, sir, I'll continue to give contributions to this organization, and wants to learn from you in the coming days

@TheSwarnim As per the issue description, please add Before/After screenshots of the HomeScreen in mobile-portrait, mobile-landscape, tablet-portrait and tablet-landscape so that we can easily compare if changes are done correctly or not.

Also in practice whenever there is a UI change we always add screenshots to PRs and when there are test cases changes then we add screenshot of tests passing.

@rt4914 rt4914 assigned TheSwarnim and unassigned rt4914 Jun 24, 2021
@TheSwarnim
Copy link
Contributor Author

Hi @TheSwarnim
Congratulations on your first Pull Request.
Please follow the first two points from the checklist given in the description section.

Thank you, sir, I'll continue to give contributions to this organization, and wants to learn from you in the coming days

@TheSwarnim As per the issue description, please add Before/After screenshots of the HomeScreen in mobile-portrait, mobile-landscape, tablet-portrait and tablet-landscape so that we can easily compare if changes are done correctly or not.

Also in practice whenever there is a UI change we always add screenshots to PRs and when there are test cases changes then we add screenshot of tests passing.

Ok sir,

Before

mobile-portrait

Screenshot_1624548660
Screenshot_1624548656

mobile-land

Screenshot_1624548678
Screenshot_1624548671

tablet-port

Screenshot_1624548782
Screenshot_1624548787

tablet-land

Screenshot_1624548825
Screenshot_1624548819

After

mobile-land

Screenshot_1624549150
Screenshot_1624549154

mobile-port

Screenshot_1624549170
Screenshot_1624549175

tablet-land

Screenshot_1624549608
Screenshot_1624549612

tablet-port

Screenshot_1624549625
Screenshot_1624549621

@anandwana001 anandwana001 assigned rt4914 and unassigned TheSwarnim Jun 25, 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.

@TheSwarnim Thanks. I have left one suggestion which applies to all files.

android:paddingStart="28dp"
android:paddingEnd="28dp">
android:paddingStart="@dimen/home_outer_margin"
android:paddingEnd="@dimen/home_outer_margin">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not use already defined values for this as it created dependencies on one another.

Create new dimens values like home_welcome_outer_padding

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok sir I'll see it and use different dimens.

@rt4914 rt4914 assigned TheSwarnim and unassigned rt4914 Jun 26, 2021
@TheSwarnim
Copy link
Contributor Author

Hello @rt4914, Sir, I've created the dimens values as per the requirement. Please review it

@rt4914 rt4914 assigned rt4914 and unassigned TheSwarnim Jun 28, 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.

Suggested changes for files which should not be changed. Otherwise everything looks good.

.idea/misc.xml Outdated Show resolved Hide resolved
.idea/runConfigurations.xml Outdated Show resolved Hide resolved
build.gradle Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned TheSwarnim and unassigned rt4914 Jun 28, 2021
@TheSwarnim
Copy link
Contributor Author

Hello @rt4914, Sir, please review it again, the error has occurred because I've not added these above files in commit mistakingly.

@rt4914 rt4914 assigned rt4914 and unassigned TheSwarnim Jun 29, 2021
@rt4914
Copy link
Contributor

rt4914 commented Jun 29, 2021

Hello @rt4914, Sir, please review it again, the error has occurred because I've not added these above files in commit mistakingly.

@TheSwarnim Thanks. Also make sure that you reply to all open comments so that the reviewer can understand what was the step taken on that particular comment.

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 enabled auto-merge (squash) June 29, 2021 09:46
@oppiabot oppiabot bot unassigned rt4914 Jun 29, 2021
@oppiabot
Copy link

oppiabot bot commented Jun 29, 2021

Unassigning @rt4914 since they have already approved the PR.

@oppiabot oppiabot bot added the PR: LGTM label Jun 29, 2021
@rt4914
Copy link
Contributor

rt4914 commented Jun 29, 2021

@TheSwarnim Please update your branch by merging with latest develop so that we can merge this PR

@rt4914 rt4914 merged commit 8dcd8de into oppia:develop Jun 29, 2021
@TheSwarnim TheSwarnim deleted the welcome_single_layout branch June 30, 2021 03:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert welcome.xml into single layout file
3 participants