-
Notifications
You must be signed in to change notification settings - Fork 191
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[GRAV-1370] [BpkOverlay] Add video overlay style #3717
Conversation
Visit https://backpack.github.io/storybook-prs/3717 to see this build running in a browser. |
@@ -61,6 +62,7 @@ const overlayTypeClassSuffixes = { | |||
[OVERLAY_TYPES.rightMedium]: 'right-medium', | |||
[OVERLAY_TYPES.rightHigh]: 'right-high', | |||
[OVERLAY_TYPES.vignette]: 'vignette', | |||
[OVERLAY_TYPES.videoOverlay]: 'video-overlay', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need to have a videoTop
and videoBottom
overlay types looking at the figma design :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, the reason I made it as one overlay is for 2 reasons:
- simplicity of having one overlay used for the homepage video component instead of 2
- since the video top overlay is only taking up 40% of the height at the top of the overlay, and the video bottom overlay is taking up 60% of the height at the bottom of the overlay, it seemed better to have a full overlay with those value since the current top/bottom overlay are taking up 100% of the height of the overlay. If I created a separate video top/bottom overlay they wouldn't be consistent with the other top/bottom overlays so it seemed better to have a separate video overlay like the vignette.
Do you think it would be better as top/bottom? Happy to change it if so :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the response!
Checking the Backpack designs and with the designers, it would be expected to be two separate types of overlay for videos :) so shouldn't ever have both the top and bottom turned on at the same time.
If we would expect that to happen, then we should talk with design to get this changed at its design level to then reflect that into the code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok makes sense, will update :)
Visit https://backpack.github.io/storybook-prs/3717 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3717 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3717 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3717 to see this build running in a browser. |
Visit https://backpack.github.io/storybook-prs/3717 to see this build running in a browser. |
JIRA: https://skyscanner.atlassian.net/browse/GRAV-1370
The new homepage hero video ad POC requires a new video overlay. This PR adds that new overlay type to
BpkOverlay
asvideoTop
andvideoBottom
.Link to Figma video overlay design: https://www.figma.com/design/yN0hFyZlKL0Jwbpi0rEKYT/Backpack-Beta?node-id=27362-1451&m=dev
Remember to include the following changes:
[KOA-123][BpkButton] Updating the colour
README.md
(If you have created a new component)README.md