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 part of #10: Hifi ContinuePlaying #605

Merged
merged 5 commits into from
Jan 17, 2020

Conversation

Luffy18346
Copy link
Contributor

@Luffy18346 Luffy18346 commented Jan 16, 2020

Explanation

Fix part of #10: Removed hardcoded height attribute and applied dimensionRatio of 16:9 to the image keeping a fixed width. So, the height of the image will be set according to the applied dimension ratio.

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The PR follows the style guide.
  • The PR does not contain any unnecessary auto-generated code from Android Studio.
  • The PR is made from a branch that's not called "develop".
  • The PR is made from a branch that is up-to-date with "develop".
  • The PR's branch is based on "develop" and not on any other branch.
  • The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

@Luffy18346 Luffy18346 changed the title changes in image dimenstion ratio in ongoing_story_card in Continue playing list ui Fix part of #10: changes in image dimenstion ratio in ongoing_story_card in Continue playing list ui Jan 16, 2020
@Luffy18346 Luffy18346 changed the title Fix part of #10: changes in image dimenstion ratio in ongoing_story_card in Continue playing list ui Fix part of #10: changes in image dimension ratio in ongoing_story_card in Continue playing list ui Jan 16, 2020
@rt4914
Copy link
Contributor

rt4914 commented Jan 16, 2020

@Luffy18346 please change the title to

Fix part of #10: Hifi ContinuePlaying

@Luffy18346 Luffy18346 changed the title Fix part of #10: changes in image dimension ratio in ongoing_story_card in Continue playing list ui Fix part of #10: Hifi ContinuePlaying Jan 16, 2020
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.

  1. The divider color should be updated to: #9C000000 the reason behind this is as follows:
    The eight character color code in mocks contains first 6 characters as color and last two characters as percentage of transparency. Now the transparency in android takes first two character and also convert 61 to hex is 9C.

  2. Bottom padding of reyclerview needs to be 172dp as per mocks.

@Luffy18346
Copy link
Contributor Author

@rt4914 , I have looked at the mock and updated the bottom padding of reyclerView to 172dp.

@rt4914
Copy link
Contributor

rt4914 commented Jan 17, 2020

@rt4914 , I have looked at the mock and updated the bottom padding of reyclerView to 172dp.

@Luffy18346 everything looks great, you just need to address two more comments:

  1. Fix part of #10: Hifi ContinuePlaying #605 (comment)
  2. Fix part of #10: Hifi ContinuePlaying #605 (review)

@Luffy18346
Copy link
Contributor Author

@rt4914, I have not changed the divider color to #9c000000 because that color is much darker than the divider color given in mock, I have checked by applying it in the app. Also, if we change this color value in color.xml then it will affect to every place where the color is used.

@rt4914
Copy link
Contributor

rt4914 commented Jan 17, 2020

@rt4914, I have not changed the divider color to #9c000000 because that color is much darker than the divider color given in mock, I have checked by applying it in the app. Also, if we change this color value in color.xml then it will affect to every place where the color is used.

Okay, even I was unsure about this. Sounds good. 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 @Luffy18346

@rt4914 rt4914 merged commit 7570940 into oppia:develop Jan 17, 2020
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.

2 participants