Conversation
mxdvl
added
Health
📶 Open Journalism
run_chromatic
mxdvl
changed the title
mxdvl
force-pushed
the
oj/islands/placeholder-simplification
branch
from
1bc7616 to
53a9b74
Compare
|
Size Change: 0 B 🆕 Total Size: 0 B |
|
I paired on this so i'll leave it to @guardian/dotcom-platform to approve, but looks good to me! |
sndrs
dismissed
their stale review
leave it team to approve (i hit the wrong button)
mxdvl
force-pushed
the
oj/islands/placeholder-simplification
branch
from
d86e21a to
20a808f
Compare
Placeholder should be handled by Islands directly, which they are doing for the most part already. Co-authored-by: Alex Sanders <[email protected]>
Co-authored-by: Alex Sanders <[email protected]>
In order to get React to reconcile the server and client DOM, we need to ensure that the initial client render matches what we rendered on the server, so we place this behind a useHydrated hook. See “The Perils of Rehydration“ https://www.joshwcomeau.com/react/the-perils-of-rehydration/#the-solution-9
mxdvl
force-pushed
the
oj/islands/placeholder-simplification
branch
from
20a808f to
b4f06c5
Compare
|
|
||
| export const useHydrated = (): boolean => { | ||
| const [hydrated, setHydrated] = useState(false); | ||
| useOnce(() => { |
There was a problem hiding this comment.
Think the useOnce is unnecessary here as its use case is to check all of the items in the dependency array to be defined before invoking the callback. In this case there is no dependency array so a regular useEffect with an empty dependency array should ensure the useEffect is called once.
There was a problem hiding this comment.
Good point, I simply went with useOnce as it was more semantic.
| }, [hasCommentsHash]); | ||
|
|
||
| useEffect(() => { | ||
| if (isServer) return; |
There was a problem hiding this comment.
useEffect shouldn't run on the server so not sure why this is required here
|
|
||
| const handlePermalink = (commentId: number) => { | ||
| if (typeof window === 'undefined') return false; | ||
| if (isServer) return false; |
There was a problem hiding this comment.
If this component shouldn't run on the server then is it better to wrap the isServer check around the component that composes it?
There was a problem hiding this comment.
Also if we do different first-pass renders on the server and client and then try to hydrate then we might get hydration errors:
There was a problem hiding this comment.
This has caused issues in GetMatchStats which can be fixed by #9007
4 participants
What does this change?
placeholderHeightprop fromIslandIslands must explicitly provide their own server-rendered placeholderWhy?
Describing the fallback should be the component's role, not the
Island's.N.B. nearly all uses of
placeholderHeightalso rendered their ownPlaceholder(cleared up here), which suggests that the API was unclear...Screenshots
N/A – identical look and behaviour