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

feat: extend Video component / image/caption spacing fix #160

Merged
merged 11 commits into from
Jun 13, 2019
Merged

feat: extend Video component / image/caption spacing fix #160

merged 11 commits into from
Jun 13, 2019

Conversation

alisonjoseph
Copy link
Member

@alisonjoseph alisonjoseph commented Jun 11, 2019

  • Extends the video component to take a local src in addition to Vimeo.
  • Updates the image spacing to work for gif and svgs
  • Updates caption spacing to work after videos

Verified

This commit was signed with the committer’s verified signature.
snyk-bot Snyk bot
@vercel
Copy link

vercel bot commented Jun 11, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://gatsby-theme-carbon-git-fork-alisonjoseph-video.carbon-design-system.now.sh

@alisonjoseph alisonjoseph changed the title feat: extend Video component feat: extend Video component / image/caption spacing fix Jun 12, 2019
@alisonjoseph alisonjoseph requested a review from vpicone June 12, 2019 00:08
vpicone
vpicone previously approved these changes Jun 12, 2019
}

// Caption after images
.gatsby-resp-image-wrapper + .#{$prefix}--caption,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this if we're targeting the img as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, because the actual image tag is nested inside so we need to target this for the sibling selector to work

@@ -2,6 +2,11 @@
margin-bottom: $spacing-08;
}

// video
.#{$prefix}--video video {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the extra element selector here? Don't think we wanna add specificity.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the component to remove the extra container divs

}
return (
<div className={`${prefix}--video`}>
{src === '' && vimeoId === '' ? (
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work if they're undefined.

I think we should just do this with proptypes, rather than rendering a paragraph tag maybe?

{src === '' && vimeoId === '' ? (
<p>Please add a vimeoId or src to your video component</p>
) : null}
{src !== '' ? <>{videoVid}</> : null}
Copy link
Contributor

Choose a reason for hiding this comment

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

{src ? videoVid : vimeoVid}

@vpicone vpicone self-requested a review June 12, 2019 15:49
@vpicone vpicone dismissed their stale review June 12, 2019 15:49

Some things might want to be changed

@vercel vercel bot temporarily deployed to staging June 13, 2019 01:40 Inactive
@alisonjoseph
Copy link
Member Author

alisonjoseph commented Jun 13, 2019

Made some updates @vpicone thoughts?

@vpicone
Copy link
Contributor

vpicone commented Jun 13, 2019

@alisonjoseph looks great, I'm just gonna update the proptypes and maybe do a quick sass module conversion then we'll be good to go

@vpicone vpicone merged commit 721029d into carbon-design-system:master Jun 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants