Repository navigation
Ads for Tag Fronts & respect speed - #8056
Merged
Merged
Conversation
Co-Authored-By: Max Duval <hi@mxdvl.com>
Revert "feat: Inject MPU states into containers on Tag fronts" This reverts commit 4d8140dd3ae5e31ebbaa340b332862e1cf5c68ab. Revert "Revert "feat: Inject MPU states into containers on Tag fronts"" This reverts commit 7bca0a66bd23c90d7a4b1c129c72959245d16e51.
|
Size Change: -45.8 kB (-7%) ✅ Total Size: 626 kB
ℹ️ View Unchanged
|
OllysCoding
marked this pull request as ready for review
June 29, 2023 11:30
mxdvl
reviewed
Jun 29, 2023
mxdvl
reviewed
Jun 29, 2023
mxdvl
reviewed
Jun 29, 2023
mxdvl
reviewed
Jun 30, 2023
mxdvl
reviewed
Jun 30, 2023
Co-Authored-By: Max Duval <hi@mxdvl.com>
Co-Authored-By: Max Duval <hi@mxdvl.com>
…/tag-fronts-containers-and-ads
Co-Authored-By: Max Duval <hi@mxdvl.com>
Co-Authored-By: Max Duval <hi@mxdvl.com>
mxdvl
reviewed
Jul 4, 2023
Comment on lines
+101
to
+102
| //@ts-expect-error – we’ve tested this manually and it’s a very helpful method | ||
| array.slice(0, count); |
Contributor
There was a problem hiding this comment.
We could have gone with augmenting the ReadonlyArray interface like so, but it feels like quite a lot for a single PR:
declare global {
interface ReadonlyArray<T> {
/**
* Returns a copy of a section of an array.
* For both start and end, a negative index can be used to indicate an offset from the end of the array.
* For example, -2 refers to the second to last element of the array.
* @param start The beginning index of the specified portion of the array.
* If start is undefined, then the slice begins at index 0.
* @param end The end index of the specified portion of the array. This is exclusive of the element at the index 'end'.
* If end is undefined, then the slice extends to the end of the array.
*/
slice<N extends 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11 | 12>(
start: 0,
end: N,
): TupleOrLess<T, N>;
}
}
mxdvl
reviewed
Jul 4, 2023
mxdvl
left a comment
Contributor
There was a problem hiding this comment.
Most wonderful use of the compiler to assert the lenght of tuples. I’m all for it, but worth checking this is not too esoteric with fellow devs <3
Comment on lines
+10
to
+12
| type Props = GroupedTrailsFastMpu & { | ||
| adIndex: number; | ||
| }; |
Contributor
There was a problem hiding this comment.
Should this not be closer to the TagFrontFastMPU?
Co-authored-by: Max Duval <max.duval@theguardian.com>
Co-authored-by: Max Duval <max.duval@theguardian.com>
…ian/dotcom-rendering into olly/tag-fronts-containers-and-ads
…ian/dotcom-rendering into olly/tag-fronts-containers-and-ads
…ian/dotcom-rendering into olly/tag-fronts-containers-and-ads
mxdvl
reviewed
Jul 4, 2023
| import { takeFirst } from './tuple'; | ||
|
|
||
| describe('takeFirst', () => { | ||
| it('Always returns the correct array length when the array is one less, the same as, or one more than n', () => { |
Contributor
There was a problem hiding this comment.
This is really asking for it.each
it.each([
// take, expected length, array
[1, 1, [0, 1]],
[1, 1, [0]],
[1, 0, []],
// etc.
])("Can take %s correctly", (count, expectedLength, array) => {
expect(takeFirst(array, count).length).toEqual(expectedLength);
}
mxdvl
reviewed
Jul 5, 2023
mxdvl
left a comment
Contributor
There was a problem hiding this comment.
I wonder if it would be better for this PR to have two commits, rather than fully squashed:
- introduction of the tuple
- all the rest of the work
Closed
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
This PR expands the existing work to support tag fronts on DCR.
This PR does 3 key things
Tuplework carried out by @mxdvl in Introduce Tuple helpers #5807speeddecided by DCR when laying out cardsTuple re-introduction
While working on the MPU injection code I found myself posed with a problem, we're only going to inject MPUs on containers which are a specific set of lengths. I wanted to be able to represent this in our type system, but didn't have an immediately obvious way of doing that. This is where I decided to use Tuples, adding in the code that @mxdvl originally wrote & expanding it to be more applicable for my use case, I was able to create really tidy definitions both in the model & in the components
speed (
slow&fast)The previous integration for TagFronts used one set of containers for all tag fronts, this PR updates this logic in order to consider the speed. There now 'shared' containers used for both slow & fast tag fronts, as well as some speed specific ones where the design calls for it. These layouts are taken directly directly from Frontend
Ads
Injected Inline Ads
This PR adds two new types at the model stage -
GroupedTrailsSlowMpu&GroupedTrailsFastMpu, representing a set of trails that should show a MPU. These types differ by speed as both the containers & number of cards are different.There are then two consuming components
TagFrontSlowMpu.tsx&TagFrontFastMpu.tsxresponsible for correctly rendering the MPU containers.Ads are only injected if
isAdFreeUseris set to false.Merchandising High
The Merchandising high slot is now added in, using the same logic as pressed fronts
Mobile ads
Mobile ads are inserted in place of injected mpus for mobile pages, this uses similar logic to pressed fronts, but has slightly fewer considerations around placement (there are no thrashers, most viewed, etc)
Some screenshots 📷
(Ads don't render locally for me so you have to imagine the blank spaces are filled!)
Desktop:
Mobile:
