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

feat(next): done - add title to pages #539

Closed

Conversation

chrisdadev13
Copy link

@chrisdadev13 chrisdadev13 commented Jul 4, 2024

What does this PR do?

Hi everyone! This PR adds document titles to each page indicating information about the page/convo. I started this task in the NextJS workspace, but when I saw that the issue was specified for Nuxt/Vue I decided to add the change here too, I'm not as good at Nuxt as I am at Next (even though the change is pretty straight forward) I would appreciate your feedback 🙏 (this is my first pr ever too)

Fixes (#215)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Chore (refactoring code, technical debt, workflow improvements)
  • Enhancement (small improvements)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist

Required

  • Read Contributing Guide
  • Self-reviewed my own code
  • Tested my code in a local environment
  • Commented on my code in hard-to-understand areas
  • Checked for warnings, there are none
  • Removed all console.logs
  • Merged the latest changes from main onto my branch with git pull origin main
  • My changes don't cause any responsiveness issues

Appreciated

  • If a UI change was made: Added a screen recording or screenshots to this PR
  • Updated the UnInbox Docs if changes were necessary

@CLAassistant
Copy link

CLAassistant commented Jul 4, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

graphite-app bot commented Jul 4, 2024

Graphite Automations

"Auto-assign PRs to author" took an action on this PR • (07/04/24)

1 assignee was added to this PR based on Rahul Mishra's automation.

@McPizza0
Copy link
Member

McPizza0 commented Jul 4, 2024

Hey @chrisdadev13
Sorry, this issue should have been re-labeled
we've stopped all work on the NuxtJS version and now only make changes to NextJS (in the /apps/web folder)
do you want to make the code changes there since you're more familiar with it?

@chrisdadev13
Copy link
Author

chrisdadev13 commented Jul 4, 2024

Hey @chrisdadev13 Sorry, this issue should have been re-labeled we've stopped all work on the NuxtJS version and now only make changes to NextJS (in the /apps/web folder) do you want to make the code changes there since you're more familiar with it?

Sure thing! I already have the changes there 🙂 I just have to prepare it

Copy link
Author

Choose a reason for hiding this comment

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

There was a bug opening convos because the "TooltipProvider" wasn't present

Copy link
Member

Choose a reason for hiding this comment

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

I fixed this issue in a recent PR, this issue was introduced in a previous PR. The fixed PR is not merged yet.
You can fix the issue in your PR for now. I will rebase this PR before merging

@chrisdadev13 chrisdadev13 changed the title feat(nuxt): done - add title to pages feat(next): done - add title to pages Jul 4, 2024
@chrisdadev13
Copy link
Author

chrisdadev13 commented Jul 4, 2024

There's a different way to add metadata to client components, but I didn't want to change too much the structure or add too many files in the PR so far. Anyway, it could have been done like they do in Dub.co and Cal.com (I think). They have two pages, a client component page and a server component page (in this case the parent component). So instead of adding a "Metadata" componenent as I did here, we would have something like:

// web/src/app/page-client.tsx
"use client";
export default function ClientPage(){
      return (
            <>
                 <h1>Client page</h1>
            </>
      )
}

// web/src/app/page.tsx
import { Metadata } from 'next'
import ClientPage from "./page-client";

export const metadata: Metadata = {
  title: '...',
  description: '...',
}
export default function Page(){
      return <ClientPage />
}

I think this way is better and if you guys are willing to review like 30(?) files change from a completely strange, I can do it like this 😀👍 @McPizza0

@McPizza0
Copy link
Member

McPizza0 commented Jul 4, 2024

Thats an interesting approach @chrisdadev13
this way would inject the metadata into the body of the page, rather than the head - i think theres implications for that

we're generally using use-client for all pages, only using SSR for login/signup
does the standard generateMetadata not work with normal clients: https://nextjs.org/docs/app/api-reference/functions/generate-metadata#generatemetadata-function

@chrisdadev13
Copy link
Author

Thats an interesting approach @chrisdadev13 this way would inject the metadata into the body of the page, rather than the head - i think theres implications for that

we're generally using use-client for all pages, only using SSR for login/signup does the standard generateMetadata not work with normal clients: https://nextjs.org/docs/app/api-reference/functions/generate-metadata#generatemetadata-function

Yes, that is true, and sadly at this time the generateMetadata only works with server components. That is why I came up with that (Metadata Component) solution.
Screenshot 2024-07-04 at 9 32 37 AM

That example I left in the comment, is how they actually recommend adding metadata on client pages. I'll try to split the PR and do it like that for few pages, if it's okay 👍 @McPizza0

@chrisdadev13
Copy link
Author

I'll close this one to make the changes more focused, here is the PR to add metadata dynamically on convo routes and statically on the page to create a new convo #542

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

4 participants