- 
                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
Conversation
8362d5a    to
    24a3b6c      
    Compare
  
    24a3b6c    to
    f046d20      
    Compare
  
    f046d20    to
    70984f5      
    Compare
  
    70984f5    to
    23b9708      
    Compare
  
    09c2f0a    to
    1faf09e      
    Compare
  
    be81d53    to
    dd2804a      
    Compare
  
    dd2804a    to
    0c7d621      
    Compare
  
    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.
I like this solution! thanks for detangling the card sizing, its definitely a confusing
| ${between.tablet.and.desktop} { | ||
| ${fixMediaWidthStyles(mediaFixedSize[tablet])} | 
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.
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 comment
The 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.
| isBetaContainer: boolean, | ||
| isSmallCard: boolean, | ||
| ) => { | ||
| if (!isBetaContainer) { | 
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.
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 comment
The 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.
| Seen on PROD (merged by @domlander 8 minutes and 55 seconds ago) Please check your changes! | 
What does this change?
Don't fix the podcast image width when the card is horizontal on desktop and the container is not scrollable small.
Simplifies the logic of fixed-size width media including podcasts.
Why?
So that the waveform sticks to the bottom of the card.
Screenshots