Atoms as islands - #3912
Merged
Merged
Atoms as islands#3912
Conversation
oliverlloyd
requested review from
a team,
JamieB-gu and
marsavar
as code owners
February 2, 2022 14:35
OllysCoding
reviewed
Feb 2, 2022
Comment on lines
-14
to
-22
| import { | ||
| QandaAtom, | ||
| GuideAtom, | ||
| ProfileAtom, | ||
| TimelineAtom, | ||
| ChartAtom, | ||
| PersonalityQuizAtom, | ||
| KnowledgeQuizAtom, | ||
| } from '@guardian/atoms-rendering'; |
Contributor
There was a problem hiding this comment.
If I'm reading this right, does this mean we were importing all these atoms on the page no matter what?! Glad to see it go!
OllysCoding
reviewed
Feb 2, 2022
OllysCoding
reviewed
Feb 2, 2022
Comment on lines
-528
to
-554
| {qandaAtoms.map((qandaAtom) => ( | ||
| <HydrateOnce rootId={qandaAtom.elementId}> | ||
| <QandaAtom | ||
| id={qandaAtom.id} | ||
| title={qandaAtom.title} | ||
| html={qandaAtom.html} | ||
| image={qandaAtom.img} | ||
| credit={qandaAtom.credit} | ||
| pillar={pillar} | ||
| likeHandler={componentEventHandler( | ||
| 'QANDA_ATOM', | ||
| qandaAtom.id, | ||
| 'LIKE', | ||
| )} | ||
| dislikeHandler={componentEventHandler( | ||
| 'QANDA_ATOM', | ||
| qandaAtom.id, | ||
| 'DISLIKE', | ||
| )} | ||
| expandCallback={componentEventHandler( | ||
| 'QANDA_ATOM', | ||
| qandaAtom.id, | ||
| 'EXPAND', | ||
| )} | ||
| /> | ||
| </HydrateOnce> | ||
| ))} |
Contributor
There was a problem hiding this comment.
I see that for Qanda atoms (no idea what these are) are no longer reporting ophan data. Is this perhaps something we can do in the wrapper to make sure we're not loosing functionality?
Contributor
Author
There was a problem hiding this comment.
The tracking requests still get sent. It's just I made these tracking props optional and then added a default inside the atoms themselves.
Contributor
There was a problem hiding this comment.
👍 Thanks for clarifying!
|
Size Change: -117 kB (-4%) Total Size: 2.82 MB
ℹ️ View Unchanged
|
Contributor
|
🏝️ #3629 |
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
3 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.
What does this change?
Here we're moving a bunch of atoms into Islands. Specifically,
We're also add version
22.2.0of@guardian/atoms-renderingwhich allows us to remove the tracking handlersWhy?
Islands are cool.
Why are we removing the tracking handlers? Because adding these on the server and then trying to hydrate them was non intuitive and added complexity. By adding a default to the atom itself we can simply not include the handler and use the fallback instead.
What are these wrapper components?
For code that's imported from an external lib it's not possible for our
Islandabstraction to dynamcally import it because we currently require island components to exist in/component. To get around this we use a simple wrapper, as shown here.