ArticleCount - #4250
Merged
Merged
ArticleCount#4250
ArticleCount#4250Conversation
…ReaderRevenueLinksIsland
Migrate braze components from App.tsx to Islands. Integrate into the layouts, and remove Portal with id `bottom-banner`. Co-Authored-By: Olly Namey <9575458+OllysCoding@users.noreply.github.com>
SlotBodyEnd and StickyBottomBanner require an ABProvider So their export is now a wrapper for this provider. Co-authored-by: Olly <OllysCoding@users.noreply.github.com>
(STOP: try to refactor before using this method) In order to avoid refactoring the existing braze logic, we introduce a hook that returns a promise. Using SWR with an immutable config ensures that the braze code is run only once.
This ensures the braze code always runs at least once on each page. > The reason that distinction is important is because > even if the slots/islands don't exist on the page, > we still want to call buildBrazeMessage currently > as it does work to clear up after the SDK if the user > has logged out or removed permissions for Braze to load. #4126 (comment)
We found that for components which consume useBraze, every render was getting a new promise from useBraze, only one of which was ever resolving. This was causing Braze messages to not be rendered correctly. We've simplified useBraze to return either a BrazeMessageInterface or nothing, instead of a Promise, which has fixed the issue. Co-authored-by: Oliver Lloyd <oliverlloyd@users.noreply.github.com>
…ReaderRevenueLinksIsland
…to oliver/new-rr-stuff
Co-authored-by: Olly <9575458+OllysCoding@users.noreply.github.com>
Co-authored-by: Oliver Lloyd <oliverlloyd@users.noreply.github.com>
…/dotcom-rendering into mxdvl/braze-swr-islands
…land' into oliver/article-cout
|
Size Change: +197 B (0%) Total Size: 1.52 MB
ℹ️ View Unchanged
|
oliverlloyd
marked this pull request as ready for review
March 15, 2022 11:46
oliverlloyd
requested review from
a team,
JamieB-gu,
iainjchambers-guardian and
marsavar
as code owners
March 15, 2022 11:46
AshCorr
approved these changes
Mar 15, 2022
AshCorr
left a comment
Member
There was a problem hiding this comment.
👍
Worth just making App.tsx an island and importing that?
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
5 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.
This PR depends on #4126
What does this change?
Moves the code to increment the article count out of
App.tsxWhy?
We're deleting it
Another micro island?
Perhaps we can optimise things by not having so many micro islands making http calls from
Page. This PR expands on this pattern for simplicity but I think we should look at rationalising all of these in a follow up PR.