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

Fixes #3564 Merge profile progress header xml #3885

Merged
merged 9 commits into from
Oct 14, 2021

Conversation

bkaur-bkj
Copy link
Contributor

@bkaur-bkj bkaur-bkj commented Oct 5, 2021

Explanation

Fixes #3564
merged portrait and landscape files of tab layout, could not merge the mobile layout files as they had some constraints and other differences too.

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
    mobile landscape before
    WhatsApp Image 2021-10-07 at 1 15 42 PM (1)

mobile landscape after changes
WhatsApp Image 2021-10-07 at 1 15 42 PM

WhatsApp Image 2021-10-07 at 1 25 32 PM (1)

WhatsApp Image 2021-10-07 at 1 25 32 PM

After changes tablet portrait

Screenshot_1633716487

After changes tablet landscape

Screenshot_1633716474

@bkaur-bkj bkaur-bkj requested a review from rt4914 as a code owner October 5, 2021 19:35
@bkaur-bkj
Copy link
Contributor Author

@rt4914 PTAL, as discussed I made a PR merging the 2 tab layout files but it says some conflicts I tried viewing them but got no idea how to resolve them and not understood the issue.

@oppiabot oppiabot bot assigned rt4914 Oct 5, 2021
@bkaur-bkj
Copy link
Contributor Author

@rt4914, I resolved the conflicts but one test is failing

@rt4914
Copy link
Contributor

rt4914 commented Oct 6, 2021

@bkaur-bkj As mentioned in the issue description please add before and after screenshot of mobile-portrait, mobile-landscape, tablet-portrait and tablet-landscape for comparison and make sure that there is not difference between before and after UI.

@rt4914 rt4914 assigned bkaur-bkj and unassigned rt4914 Oct 6, 2021
@rt4914
Copy link
Contributor

rt4914 commented Oct 6, 2021

@rt4914, I resolved the conflicts but one test is failing

@bkaur-bkj The test which is failing is an optional one and also it mostly is failing on develop too so don't worry about it in this PR.

@bkaur-bkj
Copy link
Contributor Author

@rt4914 I have added ss for mobile portrait and landscape but I tried for tablet it crashes, as I tried merging the landscape and portrait layout of tablet I removed tablet landscape layout and tried running at app on tab, it was fine with portrait but crashed in landscape so I changed the folder of the file and placed the file profile_progress_header in the layout/swa600dp/ folder. But it not crases for portrait layout too.
pls let me know what change should I do

@bkaur-bkj bkaur-bkj assigned rt4914 and unassigned bkaur-bkj Oct 7, 2021
@rt4914
Copy link
Contributor

rt4914 commented Oct 8, 2021

@rt4914 I have added ss for mobile portrait and landscape but I tried for tablet it crashes, as I tried merging the landscape and portrait layout of tablet I removed tablet landscape layout and tried running at app on tab, it was fine with portrait but crashed in landscape so I changed the folder of the file and placed the file profile_progress_header in the layout/swa600dp/ folder. But it not crases for portrait layout too. pls let me know what change should I do

@bkaur-bkj For tablet follow these steps:

  1. Uninstall the app
  2. Build -> Rebuild the project.
  3. Now install the app.
    Basically whenever we remove any xml file for tablet we need to uninstall the previous version and install the fresh build.

@rt4914
Copy link
Contributor

rt4914 commented Oct 8, 2021

@bkaur-bkj Also please merge with latest develop as there are conflicts currently.

@rt4914 rt4914 assigned bkaur-bkj and unassigned rt4914 Oct 8, 2021
@rt4914 rt4914 assigned rt4914 and unassigned bkaur-bkj Oct 8, 2021
@bkaur-bkj
Copy link
Contributor Author

@bkaur-bkj Also please merge with latest develop as there are conflicts currently.

Yes sir Done

@bkaur-bkj
Copy link
Contributor Author

@rt4914 I have added ss for mobile portrait and landscape but I tried for tablet it crashes, as I tried merging the landscape and portrait layout of tablet I removed tablet landscape layout and tried running at app on tab, it was fine with portrait but crashed in landscape so I changed the folder of the file and placed the file profile_progress_header in the layout/swa600dp/ folder. But it not crases for portrait layout too. pls let me know what change should I do

@bkaur-bkj For tablet follow these steps:

  1. Uninstall the app
  2. Build -> Rebuild the project.
  3. Now install the app.
    Basically whenever we remove any xml file for tablet we need to uninstall the previous version and install the fresh build.

yes done worked now

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.

@bkaur-bkj PTAL

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
app/src/main/res/values/dimens.xml Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned bkaur-bkj and unassigned rt4914 Oct 8, 2021
@bkaur-bkj bkaur-bkj assigned rt4914 and unassigned bkaur-bkj Oct 8, 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.

One comment left.
@bkaur-bkj PTAL

app/src/main/res/values/dimens.xml Outdated Show resolved Hide resolved
@rt4914 rt4914 assigned bkaur-bkj and unassigned rt4914 Oct 14, 2021
@bkaur-bkj
Copy link
Contributor Author

@rt4914 PTAL

@bkaur-bkj bkaur-bkj assigned rt4914 and unassigned bkaur-bkj Oct 14, 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 merged commit de345f9 into oppia:develop Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Merge profile_progress_header.xml into single xml file
2 participants