search

itsnitinr / vscode-portfolio

A VSCode themed developer portfolio built using Next.js
https://vscode-portfolio.vercel.app
MIT License
956 stars 220 forks source link

Refactor: Common out head component and placed in app.js #1

Closed saraogipraveen closed 3 years ago

saraogipraveen commented 3 years ago
vercel[bot] commented 3 years ago

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/itsnitinr/vscode-portfolio/36KKio1txg9H544Ri2u5eJqLYhNc
✅ Preview: https://vscode-portfol-git-fork-saraogipraveen-refactor-head-it-4e8bda.vercel.app

itsnitinr commented 3 years ago

Hi @saraogipraveen, this is excellent. Thanks for contributing but I think there's an issue with titleText as it is being evaluated to 'Home' irrespective of the page we're on except for the home page. Can you confirm if this happens on your end as well?

Screenshot_20210510_163133

saraogipraveen commented 3 years ago

Hi @saraogipraveen, this is excellent. Thanks for contributing but I think there's an issue with titleText as it is being evaluated to 'Home' irrespective of the page we're on except for the home page. Can you confirm if this happens on your end as well?

Screenshot_20210510_163133

@itsnitinr seems like the component's name gets changed into something else on deploy/build. There is an alternative by calling getStaticProps from each page than reading it through pageProps inside _app.js, but then it will again populate each component. Let me know if this sounds good to you else we will discard this PR. 

export async function getStaticProps(context) {
  return {
    props: {title: 'About'}, // will be passed to the page component as props
  }
}
itsnitinr commented 3 years ago

@saraogipraveen This is quite clever, nice! However, what do you think about defining getStaticProps() over importing the <Head> component into each page? Either way, thank you for the changes, and let me know which approach do you think would be better. I'll merge or revert back to the custom Head tag on each page accordingly.

saraogipraveen commented 3 years ago

@itsnitinr I think later as the project grows, we might need to anyway use getStaticProps() in pages as per requirement. This approach looks good to me. Also, importing <Head> component once is better than importing <Head> in each page

itsnitinr commented 3 years ago

Makes sense. Good call!