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 #3565 profile_progress_recently_played_story_card.xml merged into single xml file #4129

Conversation

yashbansal130
Copy link
Contributor

@yashbansal130 yashbansal130 commented Jan 24, 2022

Fixes #3565

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
Screen Before Screen After
before_port after_port
before_land after_land
Screenshot_1644648185 after-land-tab
before-port-tab after-port-tab
  • 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

@yash10019coder
Copy link
Contributor

HI @yashbansal130 thanks for creating the PR I suggest you to add screenshots in tabular form you can look at this PR for reference and @rt4914 please approve the workflow

@yashbansal130
Copy link
Contributor Author

@yash10019coder looks good?

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.

Hi @yashbansal130 you have done almost all the work I have left a comment thanks

@oppiabot
Copy link

oppiabot bot commented Jan 30, 2022

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

@yashbansal130
Copy link
Contributor Author

@yash10019coder sorry but no change request is visible to me nor any comment.

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.

@yashbansal130 why do we have to add dimensions values for layout-land swp600 and land-swp600 as their layout files are deleted so where are they used

@yashbansal130
Copy link
Contributor Author

yashbansal130 commented Feb 1, 2022

@yashbansal130 why do we have to add dimensions values for layout-land swp600 and land-swp600 as their layout files are deleted so where are they used

they have the same name as other dimen for layout_profile_recently_played_activity so what I know is that it will automatically refer to this dimen depending upon the layout is land/port/sw600dp-land/sw600dp-port

@rt4914 rt4914 assigned rt4914 and yash10019coder and unassigned yashbansal130 and rt4914 Feb 2, 2022
@rt4914
Copy link
Contributor

rt4914 commented Feb 2, 2022

Will review it once @yash10019coder has approved it.

@yash10019coder
Copy link
Contributor

@yashbansal130 why do we have to add dimensions values for layout-land swp600 and land-swp600 as their layout files are deleted so where are they used

they have the same name as other dimen for layout_profile_recently_played_activity so what I know is that it will automatically refer to this dimen depending upon the layout is land/port/sw600dp-land/sw600dp-port

ok I get that why are we declaring dimes for other configurations

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.

@yashbansal130 please update the screenshots.

@yash10019coder
Copy link
Contributor

and also resolve the merge confilcts.

@yashbansal130
Copy link
Contributor Author

yashbansal130 commented Feb 2, 2022

@yash10019coder
to get new SS, i did fetch and merge and git pull to get the updated code because i was 10 commits behind the develop branch, after doing that my emulator is not showing the whole recyclerview of the recently_played_story_card
Screenshot from 2022-02-02 21-55-03

@yash10019coder
Copy link
Contributor

This is strange I'll look into this and @rt4914 can you please approve the workflow here thanks

@yash10019coder
Copy link
Contributor

and also yash remember to assign the reviewers if you are asking something from them it lets us know that we have to see this PR

@yashbansal130
Copy link
Contributor Author

and also yash remember to assign the reviewers if you are asking something from them it lets us know that we have to see this PR

i will keep this in mind surely

@yashbansal130
Copy link
Contributor Author

This is strange I'll look into this and @rt4914 can you please approve the workflow here thanks

@yash10019coder any updates?

@yash10019coder
Copy link
Contributor

I think that your branch is not up to date with develop you must merge with latest develop thanks

@rt4914 rt4914 self-assigned this Feb 15, 2022
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.

@yashbansal130 Suggested changes.

@rishidyno @yash10019coder Please have a look at my comments and assign me once they have been finished correctly. Thanks.

app/src/main/res/values-land/dimens.xml Outdated Show resolved Hide resolved
app/src/main/res/values/dimens.xml Outdated Show resolved Hide resolved
app/src/main/res/values-land/dimens.xml Outdated Show resolved Hide resolved
app/src/main/res/values-sw600dp-land/dimens.xml Outdated Show resolved Hide resolved
app/src/main/res/values-sw600dp-land/dimens.xml Outdated Show resolved Hide resolved
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
Copy link
Contributor

@rishidyno rishidyno left a comment

Choose a reason for hiding this comment

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

@yashbansal130 LGTM, thanks.

@oppiabot
Copy link

oppiabot bot commented Feb 19, 2022

Unassigning @rishidyno since they have already approved the PR.

@yash10019coder
Copy link
Contributor

@yashbansal130 Suggested changes.

@rishidyno @yash10019coder Please have a look at my comments and assign me once they have been finished correctly. Thanks.

@yashbansal130 please always reply on comments so that we can know that you have resolved them

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 @yashbansal130 thanks

@oppiabot
Copy link

oppiabot bot commented Feb 19, 2022

Unassigning @yash10019coder since they have already approved the PR.

@rishidyno rishidyno assigned rt4914 and unassigned yashbansal130 Feb 19, 2022
@rishidyno
Copy link
Contributor

@rt4914 please aproov the workflows.

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) February 21, 2022 16:39
@oppiabot oppiabot bot unassigned rt4914 Feb 21, 2022
@oppiabot
Copy link

oppiabot bot commented Feb 21, 2022

Unassigning @rt4914 since they have already approved the PR.

@oppiabot oppiabot bot added the PR: LGTM label Feb 21, 2022
@rt4914 rt4914 merged commit 84ea593 into oppia:develop Feb 21, 2022
@yashbansal130 yashbansal130 deleted the merge_profile_progress_recently_played_story_card_into_single_xml branch February 22, 2022 04:40
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 profile_progress_recently_played_story_card.xml into single xml file
4 participants