-
-
Notifications
You must be signed in to change notification settings - Fork 510
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Reduce tearing issues #1113
Conversation
const { name, version } = useRoute().params; | ||
const { path } = useLocation(); | ||
|
||
if (!isDocPage(path) || version === LATEST_MAJOR) { | ||
if (version === LATEST_MAJOR) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was a bit of a problem as useLocation
, upon navigating from (say) /
-> /guide/...
immediately updates with the new path, but as the route wasn't loaded yet, name
& version
would be undefined
. This would result in the banner flashing upon nav.
We could correct that here, but it's better to conditionally render the entire component. OldDocsWarning
should never be loaded on anything but guide pages.
const { path } = useLocation(); | ||
const [lang] = useLanguage(); | ||
|
||
const { html, meta } = useContent([lang, path === '/' ? 'index' : path]); | ||
useTitle(meta.title); | ||
useDescription(meta.description); | ||
|
||
const hasSidebar = meta.toc !== false && isDocPage(path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant; the home page is the only one with meta.toc == false
, and the sidebar only appears on /guide/*
pages. Better to switch it to isGuide
as that's essentially what we're checking.
|
||
export function FakeSuspense(props) { | ||
this.__c = childDidSuspend; | ||
return this.state && this.state.suspended ? props.fallback : props.children; | ||
} | ||
|
||
function childDidSuspend(err) { | ||
err.then(() => this.forceUpdate()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessarily a tearing issue, but something that needed to be fixed. this.state.suspended
isn't ever truthy, and so the fallback doesn't get rendered. Most obviously seen on the home page, that first code block is gets emptied (and so collapses down) then repopulates once prism runs locally. Pretty jarring.
Tbh, I rather mindlessly copied this from #793 and didn't look too closely at it. Swapping it out for standard Suspense
from preact/compat
seems reasonable? Dunno if there's something I'm missing though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an opportunity to equalise this more in iso?
Not quite sure; my gut says this is more of an API issue. I really don't love that there's two separate hooks/contexts without clear boundaries on when to use which (besides the few bits specific to one or the other). I think this is always going to cause problems as there's always going to be some separation. Maybe there's a way to land an improvement without big changes though, will take a better look soon. |
useLocation
frompreact-iso
feels a bit problematic, as it fires significantly earlier thanuseRoute
. Don't know if it's quite to the point of being a footgun, but it causes some problems here.