Migrate standard epic to DCR #9959
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: +10.5 kB (+1%) Total Size: 756 kB
ℹ️ View Unchanged
|
This was referenced Jan 3, 2024
tomrf1
force-pushed
the
tf-migrate-epic
branch
from
fb249d5 to
4145f62
Compare
tomrf1
force-pushed
the
tf-migrate-epic
branch
from
8276972 to
ad979c2
Compare
tjmw
approved these changes
Jan 5, 2024
mxdvl
approved these changes
Jan 5, 2024
dotcom-rendering/src/components/SlotBodyEnd/ReaderRevenueEpic.tsx
Outdated
Show resolved
Hide resolved
dotcom-rendering/src/components/marketing/epics/BylineWithHeadshot.tsx
Outdated
Show resolved
Hide resolved
| url: string; | ||
| }): JSX.Element => { | ||
| const [iframeHeight, setIframeHeight] = useState(60); | ||
| const iframeRef = useRef<HTMLIFrameElement>(null); |
There was a problem hiding this comment.
@arelra made me aware that it’s probably best to useCallback for this, and make sure your useEffect has iframeRef as part of its dependencies array.
There was a problem hiding this comment.
Nice, I think I'll do this in a follow up PR though
| const markerTranslate = (goal / end) * 100 - 100; | ||
| const markerTransform = `translate3d(${markerTranslate}%, 0, 0)`; | ||
|
|
||
| return <div css={goalMarkerStyles(markerTransform)} />; |
There was a problem hiding this comment.
Not a blocker, but as per Emotion best practice, you should use the style prop for dynamic styles.
Comment on lines
+142
to
+143
| rootMargin: '-18px', | ||
| threshold: 0, |
There was a problem hiding this comment.
Can I ask why we picked a negative root margin, which shrinks the intersection area, rather than increase the threshold?
There was a problem hiding this comment.
Good question - I don't know! I think this is something to play with after this PR
Comment on lines
+13
to
+24
| window.requestAnimationFrame(() => { | ||
| setRunningTotal((prevRunningTotal) => { | ||
| const newRunningTotal = | ||
| prevRunningTotal + Math.floor(total / 100); | ||
|
|
||
| if (newRunningTotal > total) { | ||
| return total; | ||
| } | ||
|
|
||
| return newRunningTotal; | ||
| }); | ||
| }); |
There was a problem hiding this comment.
It’s curious that this does not rely on a clock, but rather on the speed at which the device is able to request animation frames…
tomrf1
force-pushed
the
tf-migrate-epic
branch
from
08ddada to
d745b77
Compare
tomrf1
force-pushed
the
tf-migrate-epic
branch
from
d745b77 to
84124be
Compare
This was referenced Jan 10, 2024
Merged
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.
Background
Currently our marketing components (epic, banner, header links) live in SDC. The SDC repo also has the server that does targeting/AB testing of marketing messages.
We want to move the client-side components from SDC to DCR.
So far, only the liveblog epic has been migrated to DCR:
#9538
Changes
This PR migrates the standard article epic.
As with the liveblog epic, the files have been copied over largely unchanged (besides import and linting fixes).
Note - this does remove the Apple Pay button feature from the epic. This was AB tested last year but is not currently in use. Migrating it to DCR would have made this PR a bit more involved so I'm leaving it out. If we change our minds later then we can do the work to add it back in. Original SDC PR.
Size
Before (from S3):
After (webpack chunk):
Screenshots from CODE
(Plenty more in storybook)