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 #3568 merged the Promoted_story_card xmls #3767

Merged
merged 5 commits into from
Sep 14, 2021

Conversation

bkaur-bkj
Copy link
Contributor

@bkaur-bkj bkaur-bkj commented Sep 4, 2021

Explanation

Fixes #3568 merges the 4 xmls of Promoted_story_card into single xml file,
could not attach screenshots as this fragment was not yet used anywhere in the app.

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).

Before ss of the landscape mode mobile

WhatsApp Image 2021-09-10 at 2 49 58 PM

After ss of the landscape mode mobile

WhatsApp Image 2021-09-10 at 1 08 38 PM (1)

Before ss of mobile portrait mode

WhatsApp Image 2021-09-10 at 2 49 58 PM (1)

After ss of landscape mode mobile

WhatsApp Image 2021-09-10 at 1 08 38 PM

@rt4914 rt4914 self-assigned this Sep 6, 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.

@bkaur-bkj PTAL thanks

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 Sep 6, 2021
@bkaur-bkj
Copy link
Contributor Author

@rt4914 sir PTAL, I have an issue in the first error you mentioned, please help me resolve it

@oppiabot oppiabot bot assigned rt4914 and unassigned bkaur-bkj Sep 6, 2021
@oppiabot
Copy link

oppiabot bot commented Sep 6, 2021

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

@rt4914 rt4914 assigned ayush0402 and bkaur-bkj and unassigned rt4914 and ayush0402 Sep 9, 2021
@bkaur-bkj
Copy link
Contributor Author

@rt4914 made the changes sir, PTAL

@oppiabot oppiabot bot assigned rt4914 and unassigned bkaur-bkj Sep 9, 2021
@oppiabot
Copy link

oppiabot bot commented Sep 9, 2021

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

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 Please add before/after screenshots of the UI in app on mobile + tablet in landscape and portrait mode.

Because I think with current changes it will crash.

app/src/main/res/layout/promoted_story_card.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
app/src/main/res/values/dimens.xml Show resolved Hide resolved
@rt4914 rt4914 assigned bkaur-bkj and unassigned rt4914 Sep 10, 2021
@bkaur-bkj
Copy link
Contributor Author

bkaur-bkj commented Sep 10, 2021 via email

@rt4914
Copy link
Contributor

rt4914 commented Sep 10, 2021

Sir as you asked me to add before and after screenshots of UI but as the promoted_story_card is not currently in use so I was not able to find it, I mentioned that while describing the PR

On Fri, Sep 10, 2021, 9:58 AM Rajat Talesra @.> wrote: @.* requested changes on this pull request. @bkaur-bkj https://github.com/bkaur-bkj Please add before/after screenshots of the UI in app on mobile + tablet in landscape and portrait mode. ------------------------------ In app/src/main/res/layout/promoted_story_card.xml <#3767 (comment)>: > @@ -80,14 +80,14 @@ android:maxLines="1" android:text="@{viewModel.promotedStory.storyName}" @./oppiaPrimaryText" - android:textSize="16sp" /> + @./promoted_story_card_text_size"/> Nit space missing at the end after " @.***/promoted_story_card_text_size" /> ------------------------------ In app/src/main/res/values-sw600dp-land/dimens.xml <#3767 (comment)>: > @@ -322,4 +322,10 @@ 48dp 100dp + + 280dp This can be removed as its same across all files. You can keep this only in values/dimens.xml fie ------------------------------ In app/src/main/res/values-sw600dp-port/dimens.xml <#3767 (comment)>: > @@ -324,5 +324,10 @@ 48dp 100dp - + + 280dp This can be removed as its same across all files. You can keep this only in values/dimens.xml fie ------------------------------ In app/src/main/res/values/dimens.xml <#3767 (comment)>: > @@ -485,4 +485,10 @@ 36dp 148dp + Introduce width here This can be removed as its same across all files. You can keep this only in values/dimens.xml file. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3767 (review)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/API6MFDYP2N3CPIUPRNYR2TUBGCO7ANCNFSM5DNP5C2Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

@bkaur-bkj You can follow these steps to see promoted stories:

  1. Go to "Test Topic 1" -> "First Story" -> "Prototype Exploration". Play this exploration a bit and then leave it in between.
  2. Go to "Fractions" -> "Matthew goes to bakery" -> "What is a Fraction?". Play this exploration a bit and then leave it in between.

Now go to Home screen and you will see both these explorations on home screen. This is "Promoted Stories"

@bkaur-bkj
Copy link
Contributor Author

@rt4914 sir PTAL , I have attached ss of mobile and it worked fine but could not attach for tablet as emulator was not responding on my laptop

@oppiabot
Copy link

oppiabot bot commented Sep 10, 2021

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

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 ff485e9 into oppia:develop Sep 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 promoted_story_card.xml into single xml file
3 participants