Youtube Atom (née MediaAtom) #46
Conversation
+1 |
src/MediaAtom/Caption.tsx
Outdated
| ); | ||
|
|
||
| type Props = { | ||
| format: Format; |
There was a problem hiding this comment.
Feels weird to have to bring caption into here when it exists in dotcom rendering? I would say that the caption used for all embeds/images/media etc should be the same? +1 on taking a Format!
|
Note on the failing compilation:
I've seen this elsewhere. Issue with TS, but can be fixed with babel plugins. Looks like fixed in TS4.0 |
src/MediaAtom/MediaMeta.tsx
Outdated
| import { pillarPalette } from './pillarPalette'; | ||
| import { Pillar } from '@guardian/types/Format'; | ||
|
|
||
| const Audio = ( |
There was a problem hiding this comment.
Are these the same media icons as in the design system? https://theguardian.design/2a1e5182b/p/3892a2-icons
There was a problem hiding this comment.
I think I just copied them to get something working, but I'll switch to the source ones!
src/MediaAtom/pillarPalette.ts
Outdated
|
|
||
| type colour = string; | ||
|
|
||
| interface PillarColours { |
There was a problem hiding this comment.
This would be useful in @guardian/types I think? Or maybe design system provides it..?
src/MediaAtom/YoutubeOverlay.tsx
Outdated
| <div | ||
| className={css` | ||
| position: absolute; | ||
| bottom: 12px; |
There was a problem hiding this comment.
Do these magic numbers relate to any particular thing that you can assign to a var/ comment to give them context?
There was a problem hiding this comment.
Thanks, I've used space here now from Source.
There are lots of questions as things stand: * do we need *all* of the variants of Caption etc? * should we just call this YoutubeAtom? * how do we handle code reuse for child components, pillar palettes etc?
nicl
force-pushed
the
nicl/media-atom
branch
from
65d3adc
to
131ceca
Compare
| }; | ||
| }; | ||
|
|
||
| const constructQuery = (query: { [key: string]: any }): string => |
There was a problem hiding this comment.
I suppose there isn't anywhere more sensible for this to live?
There was a problem hiding this comment.
Yeah, it is pretty generic code. I'm not too concerned about this being duplicated though (if it does end up being useful elsewhere), as it's only a few lines of code. Or else to refactor later as an alternative.
UPDATE: have excluded
Captionnow as the platforms may differ in this and it's a component that is shared more widely too and we'd rather not duplicate it. I've updated the picture accordingly.What does this change?
Adds (copied from DC) the Youtube/Media Atom. Note, changes have been made to:
inline icon SVGs (for now)update: use src here where possibleThere are a lot of open questions here, which I wanted to get feedback on before continuing:
do we need all of the variants of Caption etc? - I am a bit concerned with the lines of code involved in some of these components, even though they reflect (current) real features/design variationsetc?
Images