-
Notifications
You must be signed in to change notification settings - Fork 30
Add ads in galleries on both web and app #14396
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
Co-authored-by: Marjan Kalanaki <[email protected]>
Co-authored-by: Marjan Kalanaki <[email protected]>
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. |
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.
Looks great! just one suggestion for a variable name
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.
Looks good, a few comments.
<Hide until="tablet"> | ||
<DesktopAdSlot | ||
renderAds={renderAds} | ||
adSlotIndex={index} |
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.
Perhaps not one for this PR, but @marjisound we may want to look at abstracting some of this layout file into sub-components? This function is now nearly 300 lines long and hitting 13 levels of indentation in some places, which may make it difficult to read and understand, particularly when looking at a diff?
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 believe it's always better to have less lines of code in a function and refactor when needed but I second you regarding that could be in a separate PR.
Thank you @JamieB-gu for you review 🙏🏼 I addressed your comments. |
Seen on PROD (created by @marjisound and merged by @deedeeh 9 minutes and 1 second ago) Please check your changes! |
Co-authored-by: Dina Hafez [email protected]
What does this change?
This PR adds logic to insert ads in galleries after every fourth image for App. It is a continuation of the previous work on ads for galleries: #14299.
We use the
enhanceAdPlaceholders
to ensure consistency with how ads are added to other articles in the app.Since the same logic is required for both Web and App, for the web we also rely on the enhancer’s output, which adds ad placeholders to the article element list.
iOS:
Screen.Recording.2025-08-13.at.10.12.22.mov
Web:
Screen.Recording.2025-08-13.at.10.17.55.mov
This PR fixes part of #12293