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): remove white space from bottom of card #18328

Merged
merged 3 commits into from May 29, 2019

Conversation

Projects
None yet
3 participants
@studioromeo
Copy link
Contributor

commented May 21, 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?

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?

  • Yes
  • No

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:

 FAIL  src/components/select/test/basic/e2e.ts (6.289s)
  ● select: basic

    TypeError: Cannot read property 'waitForVisible' of null

Node: v10.15.3
npm: 6.4.1

fix(card): remove white space from bottom of card
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;

This comment has been minimized.

Copy link
@liamdebeasi

liamdebeasi May 22, 2019

Member

The issue here stems from img being displayed inline by default. Can you apply display: block to the img element instead of :host here?

This comment has been minimized.

Copy link
@studioromeo

studioromeo May 22, 2019

Author Contributor

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>
Screenshot 2019-05-22 at 15 15 00

<button>
Screenshot 2019-05-22 at 15 14 54

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?

This comment has been minimized.

Copy link
@liamdebeasi

liamdebeasi May 24, 2019

Member

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

liamdebeasi added some commits May 24, 2019

@liamdebeasi liamdebeasi merged commit d53e7aa into ionic-team:master May 29, 2019

1 check passed

build Workflow: build
Details
@liamdebeasi

This comment has been minimized.

Copy link
Member

commented May 29, 2019

Merged. Thank you!

@studioromeo

This comment has been minimized.

Copy link
Contributor Author

commented May 29, 2019

Awesome! Thanks 😄

@studioromeo studioromeo deleted the studioromeo:fix-card-button branch May 29, 2019

@@ -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">

This comment has been minimized.

Copy link
@adamdbradley

adamdbradley May 29, 2019

Member

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.

This comment has been minimized.

Copy link
@studioromeo

studioromeo May 29, 2019

Author Contributor

@adamdbradley thats fair. I was copying from the existing examples but I can update this in a future PR if you like?

This comment has been minimized.

Copy link
@liamdebeasi

liamdebeasi May 29, 2019

Member

@studioromeo I'm going through our tests now to update this, so I'll make sure to update the card test as well 🙂

This comment has been minimized.

Copy link
@studioromeo

studioromeo May 29, 2019

Author Contributor

Ok thanks @liamdebeasi !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.