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(card): update background to use the same as item #19602

Merged
merged 3 commits into from Nov 8, 2019

Conversation

brandyscarney
Copy link
Member

@brandyscarney brandyscarney commented Oct 9, 2019

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Build (npm run build) was run locally and any changes were pushed
  • Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Card backgrounds are transparent which causes issues

What is the new behavior?

  • uses item background but falls back to the background of the content
  • this updates it so that the background will not be transparent but white

Does this introduce a breaking change?

  • Yes
  • No

Other information

uses item background but falls back to the background of the content
@ionitron-bot ionitron-bot bot added the package: core @ionic/core package label Oct 9, 2019
@shadoWalker89
Copy link

Hi, thanks for the PR, is there a way i can check this right now before getting merged and released so i can tell you if it fixes the problem ?
Thanks.

@brandyscarney
Copy link
Member Author

@shadoWalker89 Sure! Here is a dev build, but this is based off of our 5.0.0-beta.0 release: 5.0.0-dev.201910161728.77878e2

See the changelog for what is in 5.0 so far: https://github.com/ionic-team/ionic/blob/master/CHANGELOG.md

@shadoWalker89
Copy link

shadoWalker89 commented Oct 18, 2019

So this fix will not be included in the ionic 4 ?

So this is what i did,
I installed a fresh ionic app using ionic start then chose angular then sidemenu
After that i changed in the package.json @ionic/angular from ^4.7.1 to 5.0.0-dev.201910161728.77878e2

Also i updated the src/app/app.component.html with these changes
Add content-id="main-content" to ion-menu and remove the ion-split-pane
Add id="main-content" to ion-router-outlet

I run it on the device, the card background is now fixed but the image is still flickering
I recorded a short video to demonstrate that https://www.youtube.com/watch?v=bTIZLExCb0s
If you check the other original video that i made https://www.youtube.com/watch?v=Z_qQeipYGw4 you will see that the flickering is there from the beginning as well.
So this PR fixed the background issue but not the flickering.

@blinkuz
Copy link

blinkuz commented Oct 23, 2019

@shadoWalker89 Sure! Here is a dev build, but this is based off of our 5.0.0-beta.0 release: 5.0.0-dev.201910161728.77878e2

See the changelog for what is in 5.0 so far: https://github.com/ionic-team/ionic/blob/master/CHANGELOG.md

This change will not be reflected in version 4.x?

@brandyscarney
Copy link
Member Author

brandyscarney commented Oct 23, 2019

I think there's some confusion here. This PR is only to update the card background to use the following as the default:

var(--ion-item-background, var(--ion-background-color, #fff)

Prior to this it didn't fallback to the background color of the app. However, it is currently possibly to set it to do this by setting it on the ion-card:

ion-card {
  --background: var(--ion-item-background, var(--ion-background-color, #fff);
}

Whether or not it fixes #19140 was just a possibility but this PR is not intended to fix that issue, only the issue of it not falling back to the background color. This will go into 5.x as this is not critical (it has a workaround).

@shadoWalker89
Copy link

@brandyscarney Thank your for your answer and explanation in details. But please could you check out the image problem that i mentioned in my last answer ? Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants