Dynamic data attribute for self hosted video, remove wrapper#16322
Dynamic data attribute for self hosted video, remove wrapper#16322frederickobrien wants to merge 2 commits into
Conversation
frederickobrien
added
the
maintenance
|
Hello 👋! When you're ready to run Chromatic, please apply the You will need to reapply the label each time you want to run Chromatic. |
| return ( | ||
| <figure | ||
| css={css` | ||
| margin-bottom: ${space[3]}px; |
There was a problem hiding this comment.
Just a note that this will affect self hosted video rendered on fronts, as well as articles, which I don't think is intended.
There was a problem hiding this comment.
Ah good catch, sorry. Have made it apply only in articles in 15fb6de
frederickobrien
force-pushed
the
class-name-for-long-non-yt-vids
branch
from
15fb6de to
a0bfbc1
Compare
| css={ | ||
| isInArticle | ||
| ? css` | ||
| margin-bottom: ${space[3]}px; |
There was a problem hiding this comment.
I'm not a huge fan of adding margins to the child component, instead of the parent controlling the spacing. I think the code can end up a bit messy with this approach.
I see from following the code upwards that it is not easy to add this margin to a parent container and creating a new div seems quite unnecessary. I think my preference is for the latter (and current) of these three options but happy to be overruled.
| role === 'immersive' ? 'element-video-immersive' : '' | ||
| }`} | ||
| data-component="gu-video-loop" | ||
| data-component={`gu-video-${videoStyleFormat}`} |
There was a problem hiding this comment.
Nice spot. I would run this change by the Fronts & Curation team. I think their analytics might be relying on this name
| isExternalLink: boolean; | ||
| }; | ||
| isInLoopClickTestVariant?: boolean; | ||
| isInArticle?: boolean; |
There was a problem hiding this comment.
Could this be renamed to something that describes the behaviour of this component instead of the features of the parent? It can be easier to understand how to use props and predict what they do that follow this pattern
Co-Authored-By: Pip <47357469+pippinpen@users.noreply.github.com> Co-Authored-By: Alessia Amitrano <alessia.amitranobo@gmail.com>
frederickobrien
force-pushed
the
class-name-for-long-non-yt-vids
branch
from
a0bfbc1 to
e6e6ed8
Compare
|
Please rebase this branch against
Please rebase this branch against |
3 participants
A bit of tidying around self hosted videos, this adjusts the figure's
gudata element to be dynamic like the class name is and also removes an unnecessary wrapper<div>element by moving some bottom padding to thefigureelement inside the island, where it probably belongs anyway.