fix(card): remove white space from bottom of card #18328
Conversation
When using a card with the button attribute the `<button>` element is set to display: inline-block in chrome. This causes an undesirable line of white space at the bottom of the card. This is most noticeable when using an image inside the card where the bottom of the `<img>` tag won't reach the bottom of the card. Setting the button to have `display: block` ignores this white space allowing the content to reach the end of the card.
| @@ -70,6 +70,8 @@ | |||
| @include padding(0); | |||
| @include margin(0); | |||
|
|
|||
| display: block; | |||
There was a problem hiding this comment.
The issue here stems from img being displayed inline by default. Can you apply display: block to the img element instead of :host here?
There was a problem hiding this comment.
Hm, I'm not sure to be honest. I may be misreading your comment but the <img> tag is already displayed as a block level element (https://github.com/ionic-team/ionic/blob/master/core/src/components/card/card.scss#L44-L48).
The issue currently is we have content like this
<ion-card>
<button>
<img src=".." />
</button>
</ion-card>The button element being inline block will render the space between it's closing tag and the closing ion-card type which results in a gap between the bottom of the button and the bottom of the card.
<ion-card>
<button>
If we make just the wrapping button a block level element it'll ignore the white space between closing tags and always match the height of the card which is what we want I think?
There was a problem hiding this comment.
Huh interesting... We had a similar issue on ion-img, and the solution there was to apply display: block to the img element. I took a look at the code and what you have is probably fine. 👍
|
Merged. Thank you! |
|
Awesome! Thanks 😄 |
| @@ -68,6 +68,10 @@ | |||
| </ion-card-content> | |||
| </ion-card> | |||
|
|
|||
| <ion-card button> | |||
| <img src="https://images.unsplash.com/photo-1483354483454-4cd359948304?dpr=1&auto=format&fit=crop&w=1000&q=80&cs=tinysrgb&ixid=dW5zcGxhc2guY29tOzs7Ozs%3D"> | |||
There was a problem hiding this comment.
We should avoid actual image requests here since end to end testing may or may not have downloaded the image in time for the screenshot, causing our tests to fail often. With many other tests we use a small datauri.
There was a problem hiding this comment.
@adamdbradley thats fair. I was copying from the existing examples but I can update this in a future PR if you like?
There was a problem hiding this comment.
@studioromeo I'm going through our tests now to update this, so I'll make sure to update the card test as well 🙂
There was a problem hiding this comment.
Ok thanks @liamdebeasi !
Pull request checklist
Please check if your PR fulfills the following requirements:
npm run build) was run locally and any changes were pushednpm run lint) has passed locally and any fixes were made for failuresPull request type
Please check the type of change your PR introduces:
What is the current behavior?
Issue Number: #18327
What is the new behavior?
Sets the button element to be a block element so it ignores any white space between the
</button>and</ion-card>tags. I've added a test example of this and from testing manually I couldn't see any regressions.Does this introduce a breaking change?
Other notes
I don't think this is related to this PR but in the spirit of being honest. I wasn't able to get e2e tests to run on my machine (even on the master branch) so I haven't been able to test this using the e2e suite. If anyone can help me here that'd be cool but otherwise I'm hoping the CI will save me. An example of one of the tests that fails is below:
Node: v10.15.3
npm: 6.4.1