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 #535,Fix part of #530: Material bridge theme and card-view issue fix approach -2 #537

Merged
merged 3 commits into from
Jan 21, 2020

Conversation

nikitamarysolomanpvt
Copy link
Contributor

@nikitamarysolomanpvt nikitamarysolomanpvt commented Dec 10, 2019

Explanation

To use Material Components, we need to either use a Theme.MaterialComponents.* theme or a Theme.MaterialComponents.*.Bridge theme.

See here for more info on migrating to a MaterialComponents theme (recommended) - https://github.com/material-components/material-components-android/blob/master/docs/getting-started.md#4-change-your-app-theme-to-inherit-from-a-material-components-theme

See here for more info on Bridge themes if you don't want to migrate from your current AppCompat theme - https://github.com/material-components/material-components-android/blob/master/docs/getting-started.md#bridge-themes

Additionally, we don't need to add two dependencies for Material in your :app module's build.gradle file. Below are the latest stable, beta and alpha versions of the library. Should use a single version, depending on the features vs. stability our app needs.
So i removed com.google.android.material:material:1.0.0 and used alpha.

stable: com.google.android.material:material:1.0.0
beta: com.google.android.material:material:1.1.0-beta02
alpha: com.google.android.material:material:1.2.0-alpha02
Card corner white spacing fixed for all API

Oppia Android API 19 4.4.2 (Kitkat) UI issues.

Chapter List (Mathew goes..)

Untitled

  • Material Cards UI issues

  • Drop shadow

  • Card borders

Review

Untitled

  • Material Cards UI issues

  • Drop shadow

  • Card borders

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.

@nikitamarysolomanpvt nikitamarysolomanpvt changed the title material bridge theme Fix #535,Fix part of #530: Material bridge theme and card-view issue fix Dec 10, 2019
@nikitamarysolomanpvt nikitamarysolomanpvt changed the title Fix #535,Fix part of #530: Material bridge theme and card-view issue fix Fix #535,Fix part of #530: Material bridge theme and card-view issue fix approach -2 Dec 10, 2019
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.

This and #536 looks good to me. The only thing which remain now is to decide which approach we are finalising out of the two.

@rt4914 rt4914 removed their assignment Dec 10, 2019
Copy link
Contributor

@veena14cs veena14cs left a comment

Choose a reason for hiding this comment

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

LGTM.

@BenHenning
Copy link
Member

Between this and #536, can you summarize which of the two we prefer and why?

@nikitamarysolomanpvt
Copy link
Contributor Author

nikitamarysolomanpvt commented Dec 18, 2019

Between this and #536, can you summarize which of the two we prefer and why?

@BenHenning If we don't want to migrate from our current AppCompat theme we may go with bridge themes as we may continue using our style in buttons and other widgets whereas in required widgets like card-view we may access material-component theme(Later on we may migrate to MaterialComponents theme). More than this, migrating to a MaterialComponents theme demands through testing/fixing to make sure our current UI is working as expected, If we have enough time to invest on this then only we may go ahead with MaterialComponents theme.

@BenHenning
Copy link
Member

I think we should go with the solution that doesn’t require material theme for now, and file an issue to migrate to it. I’ve had to do that on another app and it can be challenging and is definitely time consuming. I’m also not sure it will work in our benefit everywhere since the material components are aiming for GM2 styling, and our mocks are current based on GM1.

@nikitamarysolomanpvt
Copy link
Contributor Author

nikitamarysolomanpvt commented Dec 20, 2019

GM1.

@BenHenning So you want me not use bridge also ... or we may use AppCompat through bridge?
If we are using only AppCompat then the material cardviews have issues .... So you want me as if now just to raise an issue regarding card-view issue?

@BenHenning
Copy link
Member

Sorry I wasn’t clear—if using the bridge doesn’t cause issues, then that’s fine. My only concern is that cherrypicking material component pieced may have unintended side effects. That’s probably fine as long as there aren’t major inconsistencies with other elements in the app.

@rt4914
Copy link
Contributor

rt4914 commented Dec 23, 2019

Sorry I wasn’t clear—if using the bridge doesn’t cause issues, then that’s fine. My only concern is that cherrypicking material component pieced may have unintended side effects. That’s probably fine as long as there aren’t major inconsistencies with other elements in the app.

@nikitamarysolomanpvt I think what @BenHenning means here is that its fine even if you use bridge as long as its doesn't create any issues. Also, he was earlier suggesting you not to use this just because of the point that it is not a suggested to use only the specific components of material component and avoid others in which case there can be side effects.

@nikitamarysolomanpvt
Copy link
Contributor Author

Sorry I wasn’t clear—if using the bridge doesn’t cause issues, then that’s fine. My only concern is that cherrypicking material component pieced may have unintended side effects. That’s probably fine as long as there aren’t major inconsistencies with other elements in the app.

@nikitamarysolomanpvt I think what @BenHenning means here is that its fine even if you use bridge as long as its doesn't create any issues. Also, he was earlier suggesting you not to use this just because of the point that it is not a suggested to use only the specific components of material component and avoid others in which case there can be side effects.

@rt4914 I was asking your opinion on going ahead with bridge theme because I am not so sure what to reply to ben. As per our discussion ...
@BenHenning As far as I had gone through the app I didn’t find any inconsistencies, So could you please review this PR.

@BenHenning
Copy link
Member

Per our discussion during a recent team meeting, we'll be going forward with the bridge theme since that will make it easier to move to Google Material 2 style & components.

… material-cardview-issue-fix-approach-2

# Conflicts:
#	app/build.gradle
@nikitamarysolomanpvt
Copy link
Contributor Author

Per our discussion during a recent team meeting, we'll be going forward with the bridge theme since that will make it easier to move to Google Material 2 style & components.

Okay

@nikitamarysolomanpvt nikitamarysolomanpvt merged commit 7e52d4d into develop Jan 21, 2020
@nikitamarysolomanpvt nikitamarysolomanpvt deleted the material-cardview-issue-fix-approach-2 branch January 21, 2020 12:12
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.

4 participants