feat: SEO Component & Application Refactor. #68
Conversation
|
/gcbrun eventual staging example will be on https://storage.googleapis.com/staging.nodejs.dev/b3322e4/index.html |
|
✅ Staging example site looks fine to me. Let me know if there's anything to be done here. Looking forward, peace! ✌️ |
LaRuaNa
left a comment
LaRuaNa
left a comment
There was a problem hiding this comment.
All in all LGTM though wanna point out few things:
- I see couple of hard coded strings in SEO component. IMO it doesn't make sense to have the same string couple of times hard coded. It's quite error-prone if we wanna change something. Maybe would be a better approach to create a 'metaConfig' file to store all these infos or move them into 'gatsby-config'.
- Another thing I noticed is: we can actually save all the extra GraphQL requests we are doing right now in SEO component for every page. That ofc wouldn't have an impact on performance of the webpage but (if even it isn't probably noticeable) on build time. We can just import the 'metaConfig' in SEO component and request the variable infos like title in 'learn.ts' to pass them via props to SEO or at least request all the needed infos in 'learn.ts' and pass them via props to SEO so that we don't need to make a extra request. In short I'd like to keep SEO dumb :]
- I see lots of semicolons are removed again we should fix ASAP the prettierconf :]
CMIIW :]
|
@LaRuaNa That's true. In my projects, I have a base config file which avoids all the repetition and extra GraphQL queries (which are negligible). I can do a second pass on this if that's the direction we want to go in. |
|
@LaRuaNa I've just added a config file and changed all the instances of information inside the Also, resolved several merge conflicts. This is a big PR. Can you kindly look into this and merge it timely, so we can avoid more conflicts. Here if you need any help or have any questions. Looking forward, peace! ✌️ |
|
@ahmadawais nice, but fails to build can you check that please :] |
|
@LaRuaNa Everything was fine at my end until I saw the All checks passed now. Please, check! |
|
Looks good to me 👍 |
|
Fixed two new merge conflicts. 😭 |
|
Taking a look now @ahmadawais 🙂 Sorry for the delay! |
amiller-gh
left a comment
amiller-gh
left a comment
There was a problem hiding this comment.
Content looks good to me! However, let's make a choice on semicolons in the meeting next week and enforce it with prettier – this PR removes all of them from some files (ex: learn.tsx), but leaves them in others (ex: layout.tsx). My personal preference is to leave them in.
|
@amiller-gh I'm looking forward to the meeting next week would love to take responsibility for choosing formatting standards and apply that formatting everywhere. Currently, my VSCode is set up to follow the present My personal preference is the settings below but will discuss that in the next meeting: {
"semi": true,
"singleQuote": true,
"trailingComma": "es5",
"useTabs": true,
"bracketSpacing": true,
"printWidth": 120,
"tabWidth": 4
}
Thanks for merging the PR. Wohoo! 🎉 |
@ahmadawais I'm really sorry for that mate. My bad :[ Thanks for working on this 🙏 |
|
@LaRuaNa no worries at all. ☝👍👌 |
5 participants
Hey, folks! 🙌
Would love to be involved in the project. First PR here, closed the #67 since I have refactored the app and included a better reusable SEO component.
This PR does a couple of related things:
learn&404pages to includetitle,description, andauthor.Tests ran
npm start— without errorsnpm run build— without errorsLooking forward, peace! ✌️
CC: @MylesBorins kindly check this PR instead.