Feat/article with video label #708
Conversation
nikhedonia
force-pushed
the
feat/article-with-video-label
branch
5 times, most recently
from
64ea99a
to
dc1686d
Compare
packages/article/get-lead-asset.js
Outdated
| export default function getLeadAsset({ leadAsset: asset }) { | ||
| if (!asset) return { isVideo: false, leadAsset: null }; | ||
|
|
||
| const isVideo = !!asset.posterImage; |
There was a problem hiding this comment.
Is this a guarantee? will it always be populated?
There was a problem hiding this comment.
Yes, It comes directly from the graphql-endpoint.
The query resides in provider/article.
There was a problem hiding this comment.
this isn't how you should determine a video though, you have a type system?
packages/video-label/style/shared.js
Outdated
| padding: 0, | ||
| margin: 0, | ||
| position: "relative", | ||
| top: 2 |
There was a problem hiding this comment.
Can we remove this and increase the lineHeight?
There was a problem hiding this comment.
possible but then I need to add a negative marginBottom as it would introduce extra padding.
| @@ -1,24 +1,28 @@ | |||
| import React from "react"; | |||
| import PropTypes from "prop-types"; | |||
| import ArticleLabel from "@times-components/article-label"; | |||
| import VideoLabel from "@times-components/video-label"; | |||
There was a problem hiding this comment.
the native and web versions are basically the same, would be tempted to create a base version with render prop?
There was a problem hiding this comment.
This makes sense @nikhedonia
There was a problem hiding this comment.
Let's tackle this in a seperate PR
There was a problem hiding this comment.
create issue #723 to document this as a todo.
nikhedonia
force-pushed
the
feat/article-with-video-label
branch
from
f22ab31
to
5e5e13c
Compare
| ] | ||
| } | ||
| ], | ||
| "leadAsset": { |
There was a problem hiding this comment.
A video fixture should be something along the lines of http://cps-api-tnl-prod.elb.tnl-prod.ntch.co.uk/v0.8/document/sir-ken-dodd-dies-aged-90-two-days-after-marriage-32h7t6p2s
There was a problem hiding this comment.
We have a story in the backlog for video integration, just wondering if there is any specific reason / requirement for doing just the video label or is the video integration already done if I may have missed it
packages/article/get-lead-asset.js
Outdated
| export default function getLeadAsset({ type, leadAsset: asset }) { | ||
| if (!asset) return { isVideo: false, leadAsset: null }; | ||
|
|
||
| const isVideo = asset.type === 'Video'; |
| @@ -1,24 +1,28 @@ | |||
| import React from "react"; | |||
| import PropTypes from "prop-types"; | |||
| import ArticleLabel from "@times-components/article-label"; | |||
| import VideoLabel from "@times-components/video-label"; | |||
There was a problem hiding this comment.
This makes sense @nikhedonia
nikhedonia
force-pushed
the
feat/article-with-video-label
branch
from
ea468c0
to
158ef96
Compare
nikhedonia
force-pushed
the
feat/article-with-video-label
branch
from
158ef96
to
08e715d
Compare
No description provided.