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

feat(card): add card-body functional component & card-img fixes #843

Merged
merged 18 commits into from Aug 15, 2017

Conversation

Projects
None yet
4 participants
@alexsasharegan
Contributor

alexsasharegan commented Aug 14, 2017

No description provided.

@alexsasharegan alexsasharegan added this to the v1.0.0 milestone Aug 14, 2017

@alexsasharegan alexsasharegan requested review from tmorehouse, pi0 and mosinve Aug 14, 2017

@codecov-io

This comment has been minimized.

codecov-io commented Aug 14, 2017

Codecov Report

Merging #843 into 1.x will increase coverage by 0.36%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##             1.x     #843      +/-   ##
=========================================
+ Coverage   39.8%   40.16%   +0.36%     
=========================================
  Files         75       82       +7     
  Lines       2339     2360      +21     
  Branches     663      667       +4     
=========================================
+ Hits         931      948      +17     
- Misses      1245     1249       +4     
  Partials     163      163
Impacted Files Coverage Δ
lib/utils/identity.js 100% <100%> (ø)
lib/components/card-header.js 100% <100%> (ø) ⬆️
lib/components/card-footer.js 100% <100%> (ø) ⬆️
lib/utils/prefixPropName.js 100% <100%> (ø)
lib/components/card-body.js 100% <100%> (ø)
lib/utils/pluckProps.js 100% <100%> (ø) ⬆️
lib/utils/unPrefixPropName.js 100% <100%> (ø)
lib/components/card-img.js 71.42% <33.33%> (ø) ⬆️
lib/utils/upperFirst.js 66.66% <66.66%> (ø)
lib/utils/lowerFirst.js 66.66% <66.66%> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 467ec27...94c2dcb. Read the comment docs.

childNodes.push(
h(CardImg, {
props: { top: props.imgTop, bottom: props.imgBottom },
props: { top: props.imgTop, bottom: props.imgBottom, imgFluid: props.imgFluid },

This comment has been minimized.

@tmorehouse

tmorehouse Aug 14, 2017

Member

What about building the card-img first in a var, then push it in the if sections? Save on repeating some code.

This comment has been minimized.

@alexsasharegan

alexsasharegan Aug 14, 2017

Contributor

Have a look now. I changed it to accept the img slot as priority, then to dump it up top if top is specified, or if bottom is not specified (making top the default).

Apparently I have some tests to work out now.

// The order of these conditionals matter.
// We are building up the markup.
if (props.imgTop || !props.imgBottom) {

This comment has been minimized.

@tmorehouse

tmorehouse Aug 14, 2017

Member

Should this be:

if (props.img || !props.imgBottom) {

And always assume img-top if img-bottom not present?

This comment has been minimized.

@alexsasharegan

alexsasharegan Aug 14, 2017

Contributor

I shouldn't need to check props.img because of this:

let img =
  slots().img || props.img ?
    h(CardImg, {
        props: { top: props.imgTop, bottom: props.imgBottom, imgFluid: props.imgFluid },
        attrs: { src: props.img, alt: props.imgAlt }
    }) :
    undefined;

And yes, img-top if specified, or if bottom is not specified.

Alex Regan added some commits Aug 14, 2017

Alex Regan and others added some commits Aug 14, 2017

@alexsasharegan alexsasharegan changed the title from feat(card-img): add img-fluid class and img-bottom ability to feat(card): add card-body functional component & card-img fixes Aug 15, 2017

@alexsasharegan

This comment has been minimized.

Contributor

alexsasharegan commented Aug 15, 2017

Any last words, anybody? I think we're good to go here.

@mosinve

This comment has been minimized.

Member

mosinve commented Aug 15, 2017

I'll check within 15 mins

Alex Regan and others added some commits Aug 15, 2017

@alexsasharegan alexsasharegan merged commit f88ab23 into 1.x Aug 15, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@alexsasharegan alexsasharegan deleted the fix/card-img branch Aug 15, 2017

expect(body.classList.contains('card-text')).toBe(true);
it("CardBody should have assigned class", async () => {
const { app: { $refs } } = window;
expect($refs.body).toHaveClass("card-text");

This comment has been minimized.

@mosinve

mosinve Aug 15, 2017

Member

Whoa, didnt know that it can be used that way )) 👍

This comment has been minimized.

@alexsasharegan

alexsasharegan Aug 15, 2017

Contributor

Yeah, and it works across FC's and VM's! When I started with FC's, I knew testing helpers needed to be simpler to use against either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment