Dynamic data attribute for self hosted video, remove wrapper - #16322
frederickobrien wants to merge 3 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
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
There was a problem hiding this comment.
I think it would be OK to update this name provided the change is run past Ophan before its merged
a0bfbc1 to
e6e6ed8
Compare
|
Please rebase this branch against
Please rebase this branch against |
e6e6ed8 to
3967b3f
Compare
3967b3f to
0d37add
Compare
0d37add to
dd4ab5e
Compare
🚀 Image pushed to AWS ECRImage digest: 🐛 Run the image locallyThe following can be used to run the image locally: # Refer to image using the immutable digest. Find alternatives below.
IMAGE_IDENTIFIER="@sha256:abca4b1b7a6bda68c085fceede09b55f3bb4629ae58af7bbff17be70b33267b1"
# Refer to image using branch tag
# IMAGE_IDENTIFIER=":branch-class-name-for-long-non-yt-vids"
# Refer to image using build tag
# IMAGE_IDENTIFIER=":build-30845"
# Refer to image via the GitHub commit SHA tag
# IMAGE_IDENTIFIER=":sha-9aa7357a88fa0545b437190ac2b66a126798d05e"
# Set environment variables for the AWS CLI
AWS_PROFILE="<A_PROFILE_FROM_JANUS>"
AWS_DEFAULT_REGION="eu-west-1"
IMAGE_ACCOUNT_ID=$(aws ssm get-parameter --name /organisation/accounts/deployTools --query "Parameter.Value" --output text)
REGISTRY="${IMAGE_ACCOUNT_ID}.dkr.ecr.${AWS_DEFAULT_REGION}.amazonaws.com"
IMAGE="${REGISTRY}/guardian/dotcom-rendering${IMAGE_IDENTIFIER}"
# Login to AWS ECR https://docs.aws.amazon.com/AmazonECR/latest/userguide/registry_auth.html
aws ecr get-login-password | docker login --username AWS --password-stdin $REGISTRY
# Pull the image
docker pull $IMAGE
# Run the image. You'll likely need to set additional flags. See https://docs.docker.com/reference/cli/docker/container/run.
docker run $IMAGE |
| } | ||
|
|
||
| return ( | ||
| <div css={containerStyles} data-spacefinder-role="inline"> |
There was a problem hiding this comment.
Are we OK to drop the data-spacefinder-role attribute?
| role === 'immersive' ? 'element-video-immersive' : '' | ||
| }`} | ||
| data-component="gu-video-loop" | ||
| data-component={`gu-video-${videoStyleFormat}`} |
There was a problem hiding this comment.
I think it would be OK to update this name provided the change is run past Ophan before its merged
Co-Authored-By: Pip <47357469+pippinpen@users.noreply.github.com> Co-Authored-By: Alessia Amitrano <alessia.amitranobo@gmail.com>
e972417 to
787888f
Compare
5 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.