Fix #3585: Merge topic_practice_header_view.xml into single xml file. #4159

ishant904 · 2022-02-07T09:16:51Z

Fixes #3585
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).

For UI-specific PRs only

If your PR includes UI-related changes, then:

Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes
For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see RTL guide)
Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

Old SS	New SS

ishant904 · 2022-02-07T09:23:34Z

@rt4914 i have merged the landscape xmls for phone and tablet, and now it has 2 xml versions. To merge it to one xml, I think we might need to replace framelayout to constraint and take care of layout gravity. Let me know your thoughts.

ishant904 · 2022-02-10T15:08:34Z

@rt4914 @yash10019coder please do review this and let me know your thoughts

yash10019coder

@ishant904 left some comments PTAL thanks and also fix the failing tests maybe by updating the branch with the laetest develop

yash10019coder · 2022-02-12T11:52:32Z

app/src/main/res/layout-sw600dp/topic_practice_header_view.xml

@@ -37,4 +37,4 @@
        app:layout_constraintTop_toBottomOf="@+id/master_skills_text_view" />
    </androidx.constraintlayout.widget.ConstraintLayout>
  </FrameLayout>
-</layout>
+</layout>


please add a new line here for end of file

a gentle reminder

yash10019coder · 2022-02-12T11:57:35Z

app/src/main/res/layout/topic_practice_header_view.xml


    <TextView
      android:id="@+id/master_skills_text_view"
      style="@style/Heading2ViewStart"
-      android:layout_marginEnd="88dp"


why you have removed this margin end?

@yash10019coder not sure if we need it. There is nothing to the right of the textview.

please don't remove it , it is required for RTL support

still left todo

i think i have already added it in dimens file.

oppiabot · 2022-02-12T12:02:01Z

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

yash10019coder · 2022-02-14T17:12:27Z

@ishant904 please merge with latest develop and also resolve the merge confilcts.

yash10019coder

Hi @ishant904 left some comments PTAL thanks

yash10019coder · 2022-02-14T17:13:58Z

app/src/main/res/layout/topic_practice_header_view.xml


    <TextView
      android:id="@+id/master_skills_text_view"
      style="@style/Heading2ViewStart"
-      android:layout_marginEnd="88dp"


please don't remove it , it is required for RTL support

yash10019coder · 2022-02-14T17:14:13Z

app/src/main/res/layout-sw600dp/topic_practice_header_view.xml

@@ -37,4 +37,4 @@
        app:layout_constraintTop_toBottomOf="@+id/master_skills_text_view" />
    </androidx.constraintlayout.widget.ConstraintLayout>
  </FrameLayout>
-</layout>
+</layout>


a gentle reminder

oppiabot · 2022-02-14T17:17:40Z

Unassigning @yash10019coder since the review is done.

oppiabot · 2022-02-14T17:17:43Z

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

ishant904 · 2022-02-14T18:53:23Z

@yash10019coder please do review and let me know on further changes.

oppiabot · 2022-02-22T05:51:09Z

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!

…ayout

…/ishant904/oppia-android into merge_topic_practice_header_view

ishant904 · 2022-02-28T09:20:57Z

@rt4914 @yash10019coder i have merged the layout xml. please do review it.

ishant904 · 2022-03-02T20:41:03Z

@rt4914 @yash10019coder PTAL

rt4914 · 2022-03-07T18:41:50Z

Un-assigning until @yash10019coder approves it.

ishant904 · 2022-03-14T09:59:22Z

Hi @rt4914 @yash10019coder please review this and merge it if everything looks good. I am looking forward to get onboarded and work on my gsoc proposal asap. Thanks!!

yash10019coder

@ishant904 please reply to all the comments thanks

yash10019coder

@ishant904 left some comments PTAL thanks

yash10019coder · 2022-03-16T15:32:35Z

app/src/main/res/layout/topic_practice_header_view.xml


    <TextView
      android:id="@+id/master_skills_text_view"
      style="@style/Heading2ViewStart"
-      android:layout_marginEnd="88dp"


still left todo

ishant904 · 2022-03-21T07:18:47Z

@yash10019coder let me know on any other changes

rt4914 · 2022-03-24T14:19:07Z

@yash10019coder PTAL

ishant904 · 2022-03-29T08:49:53Z

@yash10019coder PTAL. Thanks

yash10019coder

Hi @ishant904 sorry for the delay, your PR looks good to me, thanks for this contribution.
@rt4914 you can now review the additional constraint layout part earlier I thought why this, but it is achieving the same results in a better way now thanks.

yash10019coder · 2022-03-29T19:28:21Z

app/src/main/res/layout/topic_practice_header_view.xml


    <TextView
      android:id="@+id/master_skills_text_view"
      style="@style/Heading2ViewStart"
-      android:layout_marginEnd="88dp"


oppiabot · 2022-03-29T19:33:34Z

Unassigning @yash10019coder since they have already approved the PR.

oppiabot · 2022-03-29T19:33:37Z

Assigning @rt4914 for code owner reviews. Thanks!

ishant904 · 2022-03-29T20:04:20Z

@yash10019coder thanks for approving. @rt4914 please do review the PR.

ishant904 · 2022-04-04T14:40:58Z

@rt4914 please do review it and let me know on any changes.

oppiabot · 2022-04-11T15:01:26Z

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!

rt4914

LGTM

Deleted topic_practice_header landscape for mobile and tablet

cdd0671

ishant904 requested a review from rt4914 as a code owner February 7, 2022 09:16

yash10019coder suggested changes Feb 12, 2022

View reviewed changes

oppiabot bot assigned ishant904 Feb 12, 2022

ishant904 requested a review from yash10019coder February 13, 2022 20:20

rt4914 assigned yash10019coder and unassigned ishant904 Feb 14, 2022

yash10019coder suggested changes Feb 14, 2022

View reviewed changes

oppiabot bot unassigned yash10019coder Feb 14, 2022

oppiabot bot assigned ishant904 Feb 14, 2022

ishant904 changed the title ~~Fix #3585: Merge topic_prctice_heder_view.xml into sigle xml file.~~ Fix #3585: Merge topic_practice_header_view.xml into single xml file. Feb 14, 2022

ishant904 and others added 2 commits February 15, 2022 00:18

added margin end in topic_practice_header for rtl support

401e3f1

Merge branch 'develop' into merge_topic_practice_header_view

07c5c03

ishant904 requested a review from yash10019coder February 14, 2022 18:51

rt4914 assigned yash10019coder and unassigned ishant904 Feb 15, 2022

oppiabot bot added stale Corresponds to items that haven't seen a recent update and may be automatically closed. and removed stale Corresponds to items that haven't seen a recent update and may be automatically closed. labels Feb 22, 2022

ishant904 added 3 commits February 22, 2022 19:56

Adding constraint layout as parent in phone xml to match the tablet l…

e113ff0

…ayout

Adding constraint layout as parent in phone xml to match the tablet l…

3771d94

…ayout

Merge branch 'merge_topic_practice_header_view' of https://github.com…

545a2ff

…/ishant904/oppia-android into merge_topic_practice_header_view

rt4914 assigned rt4914 and yash10019coder and unassigned ishant904 Mar 1, 2022

Merge branch 'develop' into merge_topic_practice_header_view

15b173a

Merge branch 'oppia:develop' into merge_topic_practice_header_view

82f4b9a

rt4914 removed their assignment Mar 7, 2022

Merge branch 'develop' into merge_topic_practice_header_view

c45d2f8

yash10019coder reviewed Mar 16, 2022

View reviewed changes

ishant904 requested a review from yash10019coder March 16, 2022 17:19

Merge branch 'develop' into merge_topic_practice_header_view

95cc5c8

yash10019coder approved these changes Mar 29, 2022

View reviewed changes

oppiabot bot unassigned yash10019coder Mar 29, 2022

oppiabot bot assigned rt4914 Mar 29, 2022

oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Apr 11, 2022

rt4914 approved these changes Apr 12, 2022

View reviewed changes

oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Apr 12, 2022

rt4914 merged commit b75c9ea into oppia:develop Apr 12, 2022

Fix #3585: Merge topic_practice_header_view.xml into single xml file. #4159

Fix #3585: Merge topic_practice_header_view.xml into single xml file. #4159

Conversation

ishant904 commented Feb 7, 2022 • edited

Essential Checklist

For UI-specific PRs only

ishant904 commented Feb 7, 2022

ishant904 commented Feb 10, 2022

yash10019coder left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oppiabot bot commented Feb 12, 2022

yash10019coder commented Feb 14, 2022 • edited

yash10019coder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oppiabot bot commented Feb 14, 2022

oppiabot bot commented Feb 14, 2022

ishant904 commented Feb 14, 2022

oppiabot bot commented Feb 22, 2022

ishant904 commented Feb 28, 2022

ishant904 commented Mar 2, 2022

rt4914 commented Mar 7, 2022

ishant904 commented Mar 14, 2022

yash10019coder left a comment

Choose a reason for hiding this comment

yash10019coder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishant904 commented Mar 21, 2022

rt4914 commented Mar 24, 2022

ishant904 commented Mar 29, 2022

yash10019coder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oppiabot bot commented Mar 29, 2022

oppiabot bot commented Mar 29, 2022

ishant904 commented Mar 29, 2022

ishant904 commented Apr 4, 2022

oppiabot bot commented Apr 11, 2022

rt4914 left a comment

Choose a reason for hiding this comment

ishant904 commented Feb 7, 2022 •

edited

yash10019coder left a comment •

edited

yash10019coder commented Feb 14, 2022 •

edited