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 the framework picker for form #133

Merged

Conversation

fulopkovacs
Copy link
Contributor

@fulopkovacs fulopkovacs commented Jan 1, 2024

Two issues has been fixed for the framework picker of the Form project:

  1. It was not working on core pages, like Overview, which is the first page the users see when they open the docs (I thought that the framework picker is not working at all because of this for a long time).
  2. If the user selected a framework and then viewed a core doc page, it went back to React, regardless of the previously selected framework.

main

framework-selector-bad.mp4

✅ This PR

framework-selector-good.mp4

Copy link

vercel bot commented Jan 1, 2024

@fulopkovacs is attempting to deploy a commit to the Tanstack Team on Vercel.

A member of the Team first needs to authorize it.

@fulopkovacs
Copy link
Contributor Author

@crutchcorn There are a couple of ideas, that that we could add to this PR:

  1. Make these changes for Store too (at first glance, the structure of Form and Store look very similar)

  2. Drop the framework string from the routes of the docs pages:

    • "/docs/latest/framework/react/quick-start" -> "/docs/latest/react/quick-start"
    • this would make the docs more consistent (the Query doc routes already look like this)
    • I think it's technically feasible, because we know all the names of the frameworks from the config.json file in the project's repo (I haven't thought deeply about this problem yet though)

Ofc these are just suggestions, changing routes are scary, plus I know much less about the projects than you do. 🤣 Wdyt?

@crutchcorn
Copy link
Member

@fulopkovacs let's do #1 for sure. I'm less sure about #2 - @tannerlinsley what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think examples wasn't supposed to be a dynamic segment in this route, because store.$version.docs.framework.$framework.$examples._index.tsx matches /store/latest/docs/framework/react/quick-start ($examples: "quick-start") and all the other framework-related doc pages.

import { repo, getBranch } from '~/routes/form'
import { DefaultErrorBoundary } from '~/components/DefaultErrorBoundary'
import { seo } from '~/utils/seo'
import { useLoaderData, useParams } from '@remix-run/react'
import { Doc } from '~/components/Doc'
import { loadDocs } from '~/utils/docs'
import { fetchRepoFile } from '~/utils/documents.server'

export const loader = async (context: LoaderFunctionArgs) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The body of the loader function is basically identical in this file and app/routes/store.$version.docs.framework.$framework.$.tsx. If we want to follow the DRY practices, we can extract the shared logic to a helper function.

Copy link

vercel bot commented Jan 9, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
tanstack-com ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 15, 2024 10:43pm

Copy link
Member

@crutchcorn crutchcorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fulopkovacs I noticed that we've seemingly fixed this by making the core docs duplicate through every framework.

core/quick-start.md -> routes/react/quick-start.md, routes/vue/quick-start.md, etc

This is a clever solution, but unfortunately one we can't move forward with. That would break our doc's SEO pretty bad, as there wouldn't be a canonical URL for core docs anymore

Instead, let's use some other kind of tool (maybe localstorage + a sidebar refactor?) to solve this issue.

@fulopkovacs
Copy link
Contributor Author

@fulopkovacs I noticed that we've seemingly fixed this by making the core docs duplicate through every framework.

core/quick-start.md -> routes/react/quick-start.md, routes/vue/quick-start.md, etc

This is a clever solution, but unfortunately one we can't move forward with. That would break our doc's SEO pretty bad, as there wouldn't be a canonical URL for core docs anymore

Instead, let's use some other kind of tool (maybe localstorage + a sidebar refactor?) to solve this

Sure, I can explore other options. We can use URLSearchParams if we want to keep using SSR, or go down the client-first localStorage route, I'll check which one requires less changes!

@crutchcorn
Copy link
Member

You're awesome, thank you for your help and receptiveness to feedback. Keep up the amazing work! 🔥

@fulopkovacs
Copy link
Contributor Author

Ok, I went with localStorage for cleaner URLs. It's pretty late here, so this implementation might have mistakes, idk... I'll review it tomorrow and request a review from you when I'm ready!

@@ -17,7 +17,7 @@ export const loader = async (context: LoaderFunctionArgs) => {
})
}

export const meta: MetaFunction = ({ data }) => {
export const meta: MetaFunction<typeof loader> = ({ data }) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed the type errors around the meta functions in the files I worked on. Most of the remaining errors of this kind are getting fixed in #135.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes #129 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome, thank you!

return seo({
title: `${data?.title} | TanStack Form Docs`,
title: `${data?.title} | TanStack Query Docs`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heads up, this needs to be changed back to Form

const framework =
params.framework || localStorage.getItem('framework') || 'react'
params.framework || localStorage.getItem(formLocalStorageKey) || 'react'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason we couldn't just keep framework as the key? It would theoretically allow us to preserve which framework is selected across multiple TanStack projects

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, no specific reason. I just assumed it's not a feature but a bug! I'm changing it back to framework.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome, thank you!

@@ -118,7 +118,7 @@ export const useReactStoreDocsConfig = () => {
const params = useParams()
const version = params.version!
const framework =
params.framework || localStorage.getItem('framework') || 'react'
params.framework || localStorage.getItem(storeLocalStorageKey) || 'react'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking on this again - could we keep it as framework rather than framework-store and framework-form?

@fulopkovacs fulopkovacs force-pushed the fix-the-framework-picker-for-form branch from 3c87c8b to 10d0d15 Compare January 13, 2024 14:06
Copy link
Member

@crutchcorn crutchcorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great fix, good work!

@crutchcorn crutchcorn merged commit 15f2363 into TanStack:main Jan 15, 2024
4 checks passed
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.

2 participants