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 #3582 :Merge topic_lessons_title.xml into single xml file #4036

Merged
merged 9 commits into from
Jan 17, 2022
Merged

Fix #3582 :Merge topic_lessons_title.xml into single xml file #4036

merged 9 commits into from
Jan 17, 2022

Conversation

shivambh12
Copy link
Contributor

@shivambh12 shivambh12 commented Dec 11, 2021 •
edited

Explanation
Fixes #3582

Essential Checklist

  1. 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: ...".)
  2. Any changes to scripts/assets files have their rationale included in the PR explanation.
  3. The PR follows the style guide.
  4. The PR does not contain any unnecessary code changes from Android Studio (reference).
  5. The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  6. The PR is assigned to the appropriate reviewers (reference).

Before Changes
Phone-port

After Changes
Phone-port(1)

Before Changes
Phone-land

After Changes
Phone-land(1)

Before Changes
Tablet-port

After Changes
Tablet-port(1)

Before Changes
Tablet-land

After Changes
Tablet-land(1)

@shivambh12 shivambh12 closed this Dec 11, 2021
@shivambh12 shivambh12 deleted the New2 branch December 11, 2021 11:02
@shivambh12 shivambh12 restored the New2 branch December 11, 2021 11:02
@shivambh12 shivambh12 reopened this Dec 11, 2021
@shivambh12 shivambh12 changed the title New2 Merge topic_lessons_title.xml into single xml file #3582 Dec 11, 2021
@shivambh12 shivambh12 changed the title Merge topic_lessons_title.xml into single xml file #3582 Merge topic_lessons_title.xml into single xml file Dec 11, 2021
@BenHenning
Copy link
Sponsor Member

Hi. As of today, 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.

Arjupta
Arjupta reviewed Dec 20, 2021
View reviewed changes
Copy link
Contributor

@Arjupta Arjupta left a comment

Choose a reason for hiding this comment

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

@shivambh12 the changes you made seems fine. You can ask for another review after resolving the conflicts

@oppiabot
Copy link

oppiabot bot commented Dec 27, 2021

Hi @shivambh12, 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 27, 2021
@shivambh12
Copy link
Contributor Author

@Arjupta How should i remove these conflicts?

@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Dec 29, 2021
@Arjupta
Copy link
Contributor

Arjupta commented Jan 1, 2022

@Arjupta How should i remove these conflicts?

  • Checkout to develop locally and pull the latest changes from upstream
  • Checkout again to your branch New2 (not a good branch name) and then merge the develop here.
  • Due to conflicts the merge won't happen completely so then resolve those conflicts in Android Studio.
  • After resolving all the conflicts stage and commit them.
  • Push the changes to New2 branch (git push origin New2) that will resolve the issue.

@shivambh12
Copy link
Contributor Author

@Arjupta When I try to pull on main branch. It says that it is up to date. Then also, there are conflicts.

@Arjupta
Copy link
Contributor

Arjupta commented Jan 7, 2022

@kritigupta45 can you help @shivambh12 here to resolve a merge conflict locally.

@kritigupta45
Copy link
Contributor

@kritigupta45 can you help @shivambh12 here to resolve a merge conflict locally.

Sure.

@shivambh12
Copy link
Contributor Author

@Arjupta I resolved the issue. Thanks for your time.

@shivambh12
Copy link
Contributor Author

@Arjupta PTAL

Arjupta
Arjupta suggested changes Jan 15, 2022
View reviewed changes
Copy link
Contributor

@Arjupta Arjupta left a comment

Choose a reason for hiding this comment

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

@shivambh12 nicely done. Take a look at few nit changes

app/src/main/res/values-land/dimens.xml Outdated Show resolved Hide resolved
app/src/main/res/layout-sw600dp-port/topic_lessons_title.xml Show resolved Hide resolved
@oppiabot oppiabot bot unassigned Arjupta Jan 15, 2022
@oppiabot
Copy link

oppiabot bot commented Jan 15, 2022

Unassigning @Arjupta since the review is done.

@oppiabot
Copy link

oppiabot bot commented Jan 15, 2022

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

@shivambh12
Copy link
Contributor Author

@Arjupta PTAL

@oppiabot oppiabot bot assigned Arjupta and unassigned shivambh12 Jan 16, 2022
@oppiabot
Copy link

oppiabot bot commented Jan 16, 2022

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

Arjupta
Arjupta approved these changes Jan 16, 2022
View reviewed changes
Copy link
Contributor

@Arjupta Arjupta left a comment

Choose a reason for hiding this comment

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

@shivambh12 can you also fix the PR title to start with Fix #issue_number : and also correct the PR description. Other than that the PR looks good to me.

@oppiabot oppiabot bot unassigned Arjupta Jan 16, 2022
@oppiabot
Copy link

oppiabot bot commented Jan 16, 2022

Unassigning @Arjupta since they have already approved the PR.

@oppiabot
Copy link

oppiabot bot commented Jan 16, 2022

Assigning @rt4914 for code owner reviews. Thanks!

@shivambh12 shivambh12 changed the title Merge topic_lessons_title.xml into single xml file Fix #3582 :Merge topic_lessons_title.xml into single xml file Jan 16, 2022
rt4914
rt4914 approved these changes Jan 17, 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.

@rt4914 rt4914 merged commit bc78d97 into oppia:develop Jan 17, 2022
@shivambh12 shivambh12 deleted the New2 branch January 18, 2022 10:44
bhaktideshmukh pushed a commit to bhaktideshmukh/oppia-android that referenced this pull request Jan 25, 2022
@shivambh12 @bhaktideshmukh
Fix oppia#3582 :Merge topic_lessons_title.xml into single xml file (o…
f2285e0 
…ppia#4036)

* topic_lessons_title port and land merged

* No such changes

* topic_lessons_title merged into 2 xml.
And, Required changes applied to dimens.xml

* No such changes

* Removed extra space

* Required Changes Applied

* Nit Changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Arjupta Arjupta Arjupta approved these changes

@rt4914 rt4914 rt4914 approved these changes

Assignees

@rt4914 rt4914

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Merge topic_lessons_title.xml into single xml file
5 participants
@shivambh12 @BenHenning @Arjupta @kritigupta45 @rt4914