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 #3583: Merge topic_practice_footer_view into single layout file. #3644

Merged
merged 14 commits into from
Aug 6, 2021
Merged

Fix #3583: Merge topic_practice_footer_view into single layout file. #3644

merged 14 commits into from
Aug 6, 2021

Conversation

rishidyno
Copy link
Contributor

@rishidyno rishidyno commented Aug 4, 2021

Explanation

Fix #3583

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.
OLD Screenshots NEW Screenshots
Screenshot_20210804_152357 Screenshot_20210804_152357
Screenshot_20210804_152412 Screenshot_20210804_152412
Screenshot_20210804_152456 Screenshot_20210804_152456
Screenshot_20210804_152510 Screenshot_20210804_152510

@rishidyno rishidyno requested a review from rt4914 as a code owner August 4, 2021 12:32
@rt4914 rt4914 assigned rishidyno and rt4914 and unassigned rishidyno Aug 4, 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.

@rishidyno There are 4 files which needs to be merged into 1.

Also as mentioned in issue please add before/after screenshots of this UI in app in mobile portrait/landscape and tablet portrait/landscape mode.

@rt4914 rt4914 assigned rishidyno and unassigned rt4914 Aug 4, 2021
@rishidyno
Copy link
Contributor Author

rishidyno commented Aug 4, 2021

@rishidyno There are 4 files which needs to be merged into 1.

Also as mentioned in issue please add before/after screenshots of this UI in app in mobile portrait/landscape and tablet portrait/landscape mode.

@rt4914 I have added Screenshots also I wanted to mention that I tried merging it into one XML file but I ran into some issues.

I added these two in mobile portrait/landscape dimens.xml beacause these two are identicle.

<dimen name="topic_practice_footer_view_margin_top">36dp</dimen>
<dimen name="topic_practice_footer_view_margin_bottom">108dp</dimen>

Also i added these two in tablet portrait/landscape dimens.xml beacause these two are identicle.

<dimen name="topic_practice_footer_view_margin_top">24dp</dimen>
<dimen name="topic_practice_footer_view_margin_bottom">0dp</dimen>

But when I tried to run the app in that activity it crashed in tablet portrait/landscape so I kept the tablet portrait and landscape file.

I could not get a complete understanding of why they crashed. But I think it might be due to other layout elements surrounding them.

I could only delete the landscape file.

Please guide me on what to do now.

@rt4914 rt4914 assigned rt4914 and unassigned rishidyno Aug 5, 2021
@rt4914
Copy link
Contributor

rt4914 commented Aug 5, 2021

@rishidyno There are 4 files which needs to be merged into 1.
Also as mentioned in issue please add before/after screenshots of this UI in app in mobile portrait/landscape and tablet portrait/landscape mode.

@rt4914 I have added Screenshots also I wanted to mention that I tried merging it into one XML file but I ran into some issues.

I added these two in mobile portrait/landscape dimens.xml beacause these two are identicle.

<dimen name="topic_practice_footer_view_margin_top">36dp</dimen>
<dimen name="topic_practice_footer_view_margin_bottom">108dp</dimen>

Also i added these two in tablet portrait/landscape dimens.xml beacause these two are identicle.

<dimen name="topic_practice_footer_view_margin_top">24dp</dimen>
<dimen name="topic_practice_footer_view_margin_bottom">0dp</dimen>

But when I tried to run the app in that activity it crashed in tablet portrait/landscape so I kept the tablet portrait and landscape file.

I could not get a complete understanding of why they crashed. But I think it might be due to other layout elements surrounding them.

I could only delete the landscape file.

Please guide me on what to do now.

@rishidyno After you have done the changes for by removing the tablet xml file also, rebuild the entire project, uninstall the previous app from tablet device and then install the app. It should work properly after that.

@rt4914 rt4914 assigned rishidyno and unassigned rt4914 Aug 5, 2021
@rishidyno
Copy link
Contributor Author

@rishidyno There are 4 files which needs to be merged into 1.
Also as mentioned in issue please add before/after screenshots of this UI in app in mobile portrait/landscape and tablet portrait/landscape mode.

@rt4914 I have added Screenshots also I wanted to mention that I tried merging it into one XML file but I ran into some issues.
I added these two in mobile portrait/landscape dimens.xml beacause these two are identicle.
<dimen name="topic_practice_footer_view_margin_top">36dp</dimen>
<dimen name="topic_practice_footer_view_margin_bottom">108dp</dimen>
Also i added these two in tablet portrait/landscape dimens.xml beacause these two are identicle.
<dimen name="topic_practice_footer_view_margin_top">24dp</dimen>
<dimen name="topic_practice_footer_view_margin_bottom">0dp</dimen>
But when I tried to run the app in that activity it crashed in tablet portrait/landscape so I kept the tablet portrait and landscape file.
I could not get a complete understanding of why they crashed. But I think it might be due to other layout elements surrounding them.
I could only delete the landscape file.
Please guide me on what to do now.

@rishidyno After you have done the changes for by removing the tablet xml file also, rebuild the entire project, uninstall the previous app from tablet device and then install the app. It should work properly after that.

Thanks a lot @rt4914 I followed what you said and I was able to merge it into a single layout file.
Now it only needs to be reviewed by you.

Copy link
Contributor

@prayutsu prayutsu left a comment

Choose a reason for hiding this comment

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

Left two comments, PTAL @rishidyno.
Also, note that if you want any reviewers to add their review, you should assign them the PR. Reviewers only add their reviews to the PRs assigned to them.

.idea/misc.xml Outdated Show resolved Hide resolved
.idea/runConfigurations.xml Outdated Show resolved Hide resolved
@rishidyno
Copy link
Contributor Author

Ptal @rt4914

Left two comments, PTAL @rishidyno.
Also, note that if you want any reviewers to add their review, you should assign them the PR. Reviewers only add their reviews to the PRs assigned to them.

Got it 🙂

@oppiabot oppiabot bot assigned rt4914 and unassigned rishidyno Aug 5, 2021
@oppiabot
Copy link

oppiabot bot commented Aug 5, 2021

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

@prayutsu prayutsu removed the request for review from BenHenning August 6, 2021 05:58
Copy link
Contributor

@prayutsu prayutsu 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 @rishidyno

@prayutsu prayutsu removed their assignment Aug 6, 2021
@rishidyno
Copy link
Contributor Author

LGTM, Thanks @rishidyno

Thanks @prayutsu

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.

@rishidyno Code is good. Just nit changes suggested.

app/src/main/res/values-sw600dp-land/dimens.xml Outdated Show resolved Hide resolved
app/src/main/res/values-sw600dp-port/dimens.xml Outdated Show resolved Hide resolved
app/src/main/res/values/dimens.xml Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned rishidyno and unassigned rt4914 Aug 6, 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.

LGTM, thanks.

@rt4914 rt4914 enabled auto-merge (squash) August 6, 2021 09:06
@oppiabot oppiabot bot added the PR: LGTM label Aug 6, 2021
@rt4914 rt4914 merged commit 4bfbcfa into oppia:develop Aug 6, 2021
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.

Merge topic_practice_footer_view.xml into single xml file
3 participants