Skip to content
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

Merged
merged 3 commits into from
Apr 9, 2024
Merged

fix: Reduce tearing issues #1113

merged 3 commits into from
Apr 9, 2024

Conversation

rschristian
Copy link
Member

@rschristian rschristian commented Apr 8, 2024

useLocation from preact-iso feels a bit problematic, as it fires significantly earlier than useRoute. Don't know if it's quite to the point of being a footgun, but it causes some problems here.

Comment on lines 53 to +55
const { name, version } = useRoute().params;
const { path } = useLocation();

if (!isDocPage(path) || version === LATEST_MAJOR) {
if (version === LATEST_MAJOR) {
Copy link
Member Author

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);
Copy link
Member Author

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.

@rschristian rschristian changed the title fix: Tearing issues fix: Reduce tearing issues Apr 9, 2024
Comment on lines -56 to -64

export function FakeSuspense(props) {
this.__c = childDidSuspend;
return this.state && this.state.suspended ? props.fallback : props.children;
}

function childDidSuspend(err) {
err.then(() => this.forceUpdate());
}
Copy link
Member Author

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.

@rschristian rschristian marked this pull request as ready for review April 9, 2024 12:16
Copy link
Member

@JoviDeCroock JoviDeCroock left a 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?

@rschristian
Copy link
Member Author

rschristian commented Apr 9, 2024

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.

@rschristian rschristian merged commit 1c1a08f into master Apr 9, 2024
5 checks passed
@rschristian rschristian deleted the fix/tearing branch April 9, 2024 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants