feat(Embed): Add component #783
Conversation
Current coverage is 100% (diff: 100%)
|
5da0b50 to
8f23956
Compare
|
Rebased to latest master, we're ready for review 👍 |
| /** Settings to configure video behavior. */ | ||
| sourceSettings: customPropTypes.every([ | ||
| customPropTypes.demand(['source']), | ||
| PropTypes.shape({ |
There was a problem hiding this comment.
I think we should flatten these settings as top level props, instead of a nested object. They are applicable only to embedded videos, but I don't think that warrants nesting these props. Let me know if you disagree and why.
| {Icon.create(icon)} | ||
| {placeholder && <img className='placeholder' src={placeholder} />} | ||
|
|
||
| <EmbedEmbed active={active} src={this.getSource()}>{children}</EmbedEmbed> |
There was a problem hiding this comment.
Given that EmbedEmbed:
- used in only one place, in
Embed - only uses props from
Embed - is not an exposed sub component
I would suggest using a method instead of a new component:
renderEmbed = () => {
const { active, children } = this.props
if (!active) return null
if (children) return <div className='embed'>{children}</div>
return (
<div className='embed'>
<iframe
allowFullScreen=''
frameBorder='0'
height='100%'
scrolling='no'
src={this.getSource()}
width='100%'
/>
</div>
)
}
render() {
return (
// ...snip
{this.renderEmbed}
)
}8f23956 to
2dd6c00
Compare
feat(Embed): Add component feat(Embed): Add component feat(Embed): Add component feat(Embed): Add component feat(Embed): Add component feat(Embed): Add tests feat(Embed): Add tests feat(Embed): Add component feat(Embed): Add component
2dd6c00 to
1d1af28
Compare
|
Removed |
| sourceAutoPlay = true, | ||
| sourceBrandedUI = false, | ||
| sourceColor = '#444444', | ||
| sourceHd = true, |
There was a problem hiding this comment.
Just a note, I'm have there same problems with demand like in #827, so I left values there instead of defaultProps.
| ]), | ||
|
|
||
| /** Specifies a url to use for embed. */ | ||
| sourceUrl: customPropTypes.every([ |
There was a problem hiding this comment.
One last update, can we remove the source prefixing from these props? I'm going for parity with the Semantic-UI core settings:
|
I'm preparing a release, so I'll go ahead and update the props :) |
|
I'll updating it now 😄 |
|
Whoops, thanks for the rapid reply! |
|
Perfect, will merge and cut a release when tests pass. |
|
Released in |
* feat(Embed): Add component feat(Embed): Add component feat(Embed): Add component feat(Embed): Add component feat(Embed): Add component feat(Embed): Add component feat(Embed): Add tests feat(Embed): Add tests feat(Embed): Add component feat(Embed): Add component * feat(Embed): Update prop names
Fixes #191