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.

Sign up for GitHub

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

Jump to bottom

Fix #3589: Deleted phone landscape xml and tablet potrait xml. #4070

Merged
merged 9 commits into from
Feb 1, 2022
Merged

Fix #3589: Deleted phone landscape xml and tablet potrait xml. #4070

merged 9 commits into from
Feb 1, 2022

Conversation

ishant904
Copy link
Contributor

@ishant904 ishant904 commented Dec 17, 2021 •
edited

Fixes #3589
Deleted phone landscape xml and tablet potrait xml.

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).
Old SS New SS
Screenshot_1639417989 Screenshot_1642015927
Screenshot_1639418725 Screenshot_1642015974
Screenshot_1639419482 Screenshot_1642016272
Screenshot_1639419467 Screenshot_1642017139

The first two are phone SS and last two are tablet SS

@github-actions
Copy link

Thanks for submitting this pull request! Some main reviewers
have taken time off for the next few weeks, so it may take a
little while before we can look at this PR. We appreciate your
patience while some of our team members recharge. We'll be fully
returning on 4 January 2021.

@ishant904 ishant904 mentioned this pull request Dec 17, 2021
Closed
@yash10019coder
Copy link
Contributor

Hi @ishant904 it is still incorrect take a look at this PR #3879

@ishant904 ishant904 changed the title Fix issue #3589 Merge topic_summary_view.xml into a single xml Fix #3589 Merge topic_summary_view.xml into a single xml Dec 18, 2021
@ishant904
Copy link
Contributor Author

@yash10019coder i have made some changes, let me know if anything else needs to be changed

@Rigzbot
Copy link

Rigzbot commented Dec 19, 2021

@ishant904 I think you linked this PR with the wrong issue.

@ishant904
Copy link
Contributor Author

@Rigzbot sorry I have updated the PR

@yash10019coder
Copy link
Contributor

Hi @ishant904 it is still incorrect take a look at this PR #3879

@ishant904 still incorrect take a look at this pr

@ishant904 ishant904 changed the title Fix #3589 Merge topic_summary_view.xml into a single xml Fix #3859: Deleted phone landscape xml and tablet potrait xml. Dec 21, 2021
@ishant904 ishant904 changed the title Fix #3859: Deleted phone landscape xml and tablet potrait xml. Fix #3589: Deleted phone landscape xml and tablet potrait xml. Dec 21, 2021
@ishant904
Copy link
Contributor Author

@yash10019coder Please correct me if I am wrong, #3589 is the issue and the PR #3879 which you mentioned is referring to #3859 in the title. If you can please lead me to the particular information which needs to be changed.

@yash10019coder
Copy link
Contributor

@yash10019coder Please correct me if I am wrong, #3589 is the issue and the PR #3879 which you mentioned is referring to #3859 in the title. If you can please lead me to the particular information which needs to be changed.

your work is correct you just need to display the screenshots in table like format.
like done in this PR #3879

@yash10019coder
Copy link
Contributor

You can view this blog post for reference

@ishant904
Copy link
Contributor Author

@yash10019coder thanks for pointing that out. I have made some changes. Let me know if any changes are required

@oppiabot
Copy link

oppiabot bot commented Dec 29, 2021

Hi @ishant904, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Dec 29, 2021
@oppiabot oppiabot bot closed this Jan 5, 2022
@ishant904
Copy link
Contributor Author

@yash10019coder I was out for couple of weeks and now the PR is closed. Any way to reopen it or should I create a new one?

@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Jan 12, 2022
@yash10019coder
Copy link
Contributor

No problem @ishant904 opened the PR

@ishant904
Copy link
Contributor Author

thanks @yash10019coder . I have resolved conflict, please do review the PR

yash10019coder
yash10019coder reviewed Jan 12, 2022
View reviewed changes
Copy link
Contributor

@yash10019coder yash10019coder left a comment

Choose a reason for hiding this comment

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

@ishant904 reformat the code and also try to remove swp600 portrait

app/src/main/res/values/dimens.xml Show resolved Hide resolved
app/src/main/res/values-sw600dp-land/dimens.xml Outdated
<!-- AudioLanguageFragment -->
<dimen name="audio_language_recycler_view_padding_bottom">132dp</dimen>

Copy link
Contributor

Choose a reason for hiding this comment

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

same goes here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed newlines

@yash10019coder
Copy link
Contributor

and @rt4914 can you please approve the workflow

@FareesHussain
Copy link
Contributor

@FareesHussain i see couple of test failed. Let me know the further actions. Also I unassigned myself by mistake.

Try to read the failure checks, look for the key words failed error and look for the reason why it fails. Let me know if you are unable to figure out the issue

@ishant904
Copy link
Contributor Author

ishant904 commented Jan 25, 2022 •
edited

@rt4914 @yash10019coder @FareesHussain Please do review it.

@ishant904 ishant904 requested a review from rt4914 January 25, 2022 20:43
@oppiabot oppiabot bot assigned FareesHussain and rt4914 and unassigned ishant904 Jan 25, 2022
@oppiabot
Copy link

oppiabot bot commented Jan 25, 2022

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

yash10019coder
yash10019coder suggested changes Jan 26, 2022
View reviewed changes
Copy link
Contributor

@yash10019coder yash10019coder left a comment

Choose a reason for hiding this comment

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

@ishant904 I have mentioned some changes PTAL thanks

app/src/main/res/values-sw600dp-land/dimens.xml Show resolved Hide resolved
app/src/main/res/values/dimens.xml Show resolved Hide resolved
@yash10019coder
Copy link
Contributor

and @rt4914 please approve the workflows

@oppiabot
Copy link

oppiabot bot commented Jan 26, 2022

Unassigning @yash10019coder since the review is done.

@oppiabot
Copy link

oppiabot bot commented Jan 26, 2022

Hi @ishant904, it looks like some changes were requested on this pull request by @yash10019coder. PTAL. Thanks!

@ishant904
Copy link
Contributor Author

@FareesHussain @rt4914 @yash10019coder I looked into one of the failed CI checks and it says that gradle daemon disappered unexpectedly. Any suggestion?

@FareesHussain
Copy link
Contributor

re-ran the jobs lets see if it passes

rt4914
rt4914 approved these changes Jan 28, 2022
View reviewed changes
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.

@oppiabot oppiabot bot added the PR: LGTM label Jan 28, 2022
@ishant904
Copy link
Contributor Author

@yash10019coder @rt4914 @FareesHussain one of the CI test is getting failed repeatedly. Looking into error msg, it states the gradle daemon was killed/crashed while running the build. I think it maybe because of low memeory allocated. Let me know your thoughts.

yash10019coder
yash10019coder approved these changes Jan 30, 2022
View reviewed changes
Copy link
Contributor

@yash10019coder yash10019coder left a comment

Choose a reason for hiding this comment

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

LGTM @ishant904 Sorry for the delay, I think that it's a flaky test I'll rerun the ci checks.

@ishant904
Copy link
Contributor Author

Test cases passed. Thanks @yash10019coder

@rt4914 rt4914 merged commit f9d7f19 into oppia:develop Feb 1, 2022
@ishant904 ishant904 deleted the merge-topic-summary branch February 1, 2022 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rt4914 rt4914 rt4914 approved these changes

@yash10019coder yash10019coder yash10019coder approved these changes

Assignees

@ishant904 ishant904

@FareesHussain FareesHussain

Labels
PR: LGTM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Merge topic_summary_view.xml into single xml file
5 participants
@ishant904 @yash10019coder @Rigzbot @rt4914 @FareesHussain