-
Notifications
You must be signed in to change notification settings - Fork 30
Extend podcast image size in Flexible General half-width slot #14753
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
Changes from all commits
909df92
0c7d621
41d4225
0c3dc4f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,14 +9,6 @@ const mediaFixedSize = { | |
| medium: 125, | ||
| }; | ||
|
|
||
| type MediaFixedSize = keyof typeof mediaFixedSize; | ||
|
|
||
| export type MediaFixedSizeOptions = { | ||
| mobile?: MediaFixedSize; | ||
| tablet?: MediaFixedSize; | ||
| desktop?: MediaFixedSize; | ||
| }; | ||
|
|
||
| export type MediaPositionType = 'left' | 'top' | 'right' | 'bottom' | 'none'; | ||
| export type MediaSizeType = | ||
| | 'small' | ||
|
|
@@ -36,18 +28,17 @@ export type MediaSizeType = | |
| type Props = { | ||
| children: React.ReactNode; | ||
| mediaSize: MediaSizeType; | ||
| mediaFixedSizes?: MediaFixedSizeOptions; | ||
| mediaType?: CardMediaType; | ||
| mediaPositionOnDesktop: MediaPositionType; | ||
| mediaPositionOnMobile: MediaPositionType; | ||
| fixImageWidth: boolean; | ||
| /** | ||
| * Forces hiding the image overlay added to pictures & slideshows on hover. | ||
| * This is to allow hiding the overlay on slideshow carousels where we don't | ||
| * want it to be shown whilst retaining it for existing slideshows. | ||
| */ | ||
| hideImageOverlay?: boolean; | ||
| isBetaContainer?: boolean; | ||
| isBetaContainer: boolean; | ||
| isSmallCard: boolean; | ||
| padMedia?: boolean; | ||
| }; | ||
|
|
||
|
|
@@ -86,9 +77,13 @@ const mediaPaddingStyles = ( | |
| */ | ||
| const flexBasisStyles = ({ | ||
| mediaSize, | ||
| mediaType, | ||
| isSmallCard, | ||
| isBetaContainer, | ||
| }: { | ||
| mediaSize: MediaSizeType; | ||
| mediaType: CardMediaType; | ||
| isSmallCard: boolean; | ||
| isBetaContainer: boolean; | ||
| }): SerializedStyles => { | ||
| if (!isBetaContainer) { | ||
|
|
@@ -119,6 +114,15 @@ const flexBasisStyles = ({ | |
| } | ||
| } | ||
|
|
||
| if (mediaType === 'podcast' && !isSmallCard) { | ||
| return css` | ||
| flex-basis: 120px; | ||
| ${from.desktop} { | ||
| flex-basis: 168px; | ||
| } | ||
| `; | ||
| } | ||
|
|
||
| switch (mediaSize) { | ||
| default: | ||
| case 'small': | ||
|
|
@@ -150,6 +154,7 @@ const flexBasisStyles = ({ | |
| `; | ||
| } | ||
| }; | ||
|
|
||
| /** | ||
| * Below tablet, we fix the size of the media and add a margin around it. | ||
| * The corresponding content flex grows to fill the space. | ||
|
|
@@ -163,43 +168,48 @@ const fixMediaWidthStyles = (width: number) => css` | |
| align-self: flex-start; | ||
| `; | ||
|
|
||
| const fixMediaWidth = ({ | ||
| mobile, | ||
| tablet, | ||
| desktop, | ||
| }: MediaFixedSizeOptions) => css` | ||
| ${mobile && | ||
| css` | ||
| const fixMobileMediaWidth = ( | ||
| isBetaContainer: boolean, | ||
| isSmallCard: boolean, | ||
| ) => { | ||
| if (!isBetaContainer) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, this will make it easier to rip out once we fully discontinue old containers There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That was my thinking. It should be easy to delete this old code once the time comes. |
||
| return css` | ||
| ${until.tablet} { | ||
| ${fixMediaWidthStyles(mediaFixedSize.medium)} | ||
| } | ||
| `; | ||
| } | ||
|
|
||
| const size = isSmallCard ? mediaFixedSize.tiny : mediaFixedSize.small; | ||
|
|
||
| return css` | ||
| ${until.tablet} { | ||
| ${fixMediaWidthStyles(mediaFixedSize[mobile])} | ||
| ${fixMediaWidthStyles(size)} | ||
| } | ||
| `} | ||
| ${tablet && | ||
| css` | ||
| ${between.tablet.and.desktop} { | ||
| ${fixMediaWidthStyles(mediaFixedSize[tablet])} | ||
|
Comment on lines
-179
to
-180
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like we no longer account for a size change between tablet and desktop. Is this intentional? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, we didn't make a distinction here: https://github.com/guardian/dotcom-rendering/pull/14753/files#diff-fbf8297916d3e244f5c5288b6fe289a3ac99c9a22d6211ee0d741c6c0c27da8bL593. I've also tested that tablet looks correct. |
||
| } | ||
| `} | ||
| ${desktop && | ||
| css` | ||
| ${from.desktop} { | ||
| ${fixMediaWidthStyles(mediaFixedSize[desktop])} | ||
| `; | ||
| }; | ||
|
|
||
| const fixDesktopMediaWidth = () => { | ||
| return css` | ||
| ${from.tablet} { | ||
| ${fixMediaWidthStyles(mediaFixedSize.small)} | ||
| } | ||
| `} | ||
| `; | ||
| `; | ||
| }; | ||
|
|
||
| export const MediaWrapper = ({ | ||
| children, | ||
| mediaSize, | ||
| mediaFixedSizes = { mobile: 'medium' }, | ||
| mediaType, | ||
| mediaPositionOnDesktop, | ||
| mediaPositionOnMobile, | ||
| fixImageWidth, | ||
| hideImageOverlay, | ||
| isBetaContainer = false, | ||
| isBetaContainer, | ||
| isSmallCard, | ||
| padMedia, | ||
| }: Props) => { | ||
| const isHorizontalOnMobile = | ||
| mediaPositionOnMobile === 'left' || mediaPositionOnMobile === 'right'; | ||
| const isHorizontalOnDesktop = | ||
| mediaPositionOnDesktop === 'left' || mediaPositionOnDesktop === 'right'; | ||
|
|
||
|
|
@@ -209,10 +219,13 @@ export const MediaWrapper = ({ | |
| (mediaType === 'slideshow' || | ||
| mediaType === 'picture' || | ||
| mediaType === 'youtube-video' || | ||
| mediaType === 'loop-video') && | ||
| mediaType === 'loop-video' || | ||
| mediaType === 'podcast') && | ||
| isHorizontalOnDesktop && | ||
| flexBasisStyles({ | ||
| mediaSize, | ||
| mediaType, | ||
| isSmallCard, | ||
| isBetaContainer, | ||
| }), | ||
| mediaType === 'avatar' && | ||
|
|
@@ -227,7 +240,10 @@ export const MediaWrapper = ({ | |
| display: none; | ||
| } | ||
| `, | ||
| fixImageWidth && fixMediaWidth(mediaFixedSizes), | ||
| isHorizontalOnMobile && | ||
| mediaType !== 'podcast' && | ||
| fixMobileMediaWidth(isBetaContainer, isSmallCard), | ||
| isSmallCard && fixDesktopMediaWidth(), | ||
| isHorizontalOnDesktop && | ||
| css` | ||
| ${from.tablet} { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.