Add support for podcast image i n Picture.tsx #12568
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +21 B (0%) Total Size: 921 kB ℹ️ View Unchanged
|
# Conflicts: # dotcom-rendering/src/layouts/StandardLayout.tsx
# Conflicts: # dotcom-rendering/src/components/Picture.tsx # dotcom-rendering/src/layouts/StandardLayout.tsx
# Conflicts: # dotcom-rendering/src/components/ArticleMeta.web.tsx
|
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. |
github-actions
bot
removed
the
run_chromatic
SiAdcock
reviewed
Oct 21, 2024
| isMainMedia, | ||
| isLightbox, | ||
| orientation, | ||
| }), |
There was a problem hiding this comment.
If we're going to add more customisation to the Picture component, do we want to do it like this, where we add an override that says anything goes, or do we want to add a new flag for isPodcastCover or something like that?
Reason I ask is because of the disclaimer at the top of decideImageWidths:
All business logic for image sizing is contained in this one function. This is the source of truth.
With this change, this is no longer the case.
I'll admit it doesn't feel sustainable to have a function that receives a growing series of arguments to account for a possibly endless list of weird and wonderful new use cases. It also doesn't feel good for that function to canonise some widths as "source of truth" and for everything else, there's a custom role. And since this podcast cover image has been around for at least 8 years and is canonised in CAPI and tag manager, maybe dotcom-rendering needs to bite the bullet and accept it as a thing also.
PS I'm away until Thursday so please overrule me if, in general, there's disagreement on this point. I don't want my absence to be a blocker.
There was a problem hiding this comment.
The extra argument way you suggested seems to be how many of these components work in DCR. Also I don't want to break that contract about the source of truth. I will change it happily to use a prop through the Picture component as suggested.
… the source of truth for image widths
… the source of truth for image widths
arelra
reviewed
Oct 23, 2024
SiAdcock
approved these changes
Oct 24, 2024
arelra
approved these changes
Oct 24, 2024
oliverabrahams
changed the title
github-actions
bot
removed
the
run_chromatic
sndrs
approved these changes
Oct 24, 2024
|
Seen on PROD (created by @oliverabrahams and merged by @sndrs 8 minutes and 24 seconds ago) Please check your changes! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this change?
Adds support for the podcast cover image to the Picture component.
Why?
Migrating the audio page from FE to DCR.
This PR has a slight change in the Props for the
<Picture/>component. It updates the argrole: PictureRoleType