-
Notifications
You must be signed in to change notification settings - Fork 33
removed chakra-ui & revamped website #171
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for devsindia ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM accept a few changes here & there which if added right now will help in easier reviewing & maintenance later on.
import styles from "./styles.module.scss"; | ||
import React from "react"; | ||
|
||
interface Props { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use Type Aliases instead of Interfaces just for following a uniform standard practice across the project's source code?
I used Type Aliases elsewhere mainly because I don't need them to be extended & composed for say a third-party user. Generally speaking for internal usages (as in not to be imported in to a third-party project), Type Aliases are the way to go.
text: "Button Text", | ||
}; | ||
|
||
function Button(props: Props) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The props
parameter can be directly destructured (and is the recommended approach to writing Functional Components right now).
Here's an example:
function Button({ props }: PropTypes) {
return <>Some JSX Element</>
}
return <>{button}</>; | ||
} | ||
|
||
Button.defaultProps = defaultProps; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving forward with Functional Components, using defaultProps
is redundant. You can now directly default values to the props like the example below:
function Button({
isLink = false,
isExternal = false,
href = "",
onClick: () => {},
text = "Button Text"
}: PropTypes) {
return <>Some JSX Elemet</>
}
This is way MORE readable, has less code to write & follows the uniform standard across the rest of the source code.
For more information on the same, refer to the official documentations.
import Link from "next/link"; | ||
|
||
import styles from "./footer.module.scss"; | ||
import React, { useState } from "react"; | ||
|
||
function FooterSection() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the "newsletter" section of the footer area right? Can you add a TODO comment here for future reference? It's easier to lookup for TODO comments across all the files in the source code & fix the issues later on. My justification for doing so is because we need to setup some mailing list service before this function can hook in to it.
Like add a comment on the lines of:
TODO: Setup a Mailchimp account & see if it works.
data: StatsData | ||
} | ||
function Stats(props: Props) { | ||
const { dailySubredditViews, subredditMembers, discordServerMembers } = props.data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Destructure the props
like the following:
function Stats({
dailySubredditViews,
subredditMembers,
discordServerMembers
}: PropTypes) {
return <>Return some JSX Element</>
}
For reference, see a comment above where I shared the official documentations for the same as well.
|
||
function IndexPage() { | ||
|
||
interface Props { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Type Aliases instead of Interfaces.
statsData: StatsData | ||
} | ||
|
||
function IndexPage(props: Props) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Destructure the props as a function parameter for readability & following a uniform standard practice across the source code.
); | ||
} | ||
|
||
export async function getServerSideProps(context: any) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like how we're using any
for passing the Type Alias, it defeats the purpose of using TypeScript then. 😬
Can't we rewrite this as such:
export async function getServerSideProps({ res }: { res: NextResponse }) {
// ...rest of the code
}
The NextResponse
Type Alias exists over here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I think rather than use some hacks & "uneasy code" (because getServerSideProps
will unnecessarily increase the TTL of this index.tsx
file), we can use SWR. That way not only can we share the latest information about the community but keep load times of the page snappy as well.
So each page
component in a Next.js project that uses getServerSideProps
renders the page on request & not statically! I say its better to serve a single static page (i.e a HTML page) & let React do the heavy-lifting of client-side data fetching using useSWR
.
This way our single page landing page will have a lower TTL (which directly equates to better SEO) & better cache control of the data on client-side rather than on server-side.
@@ -0,0 +1,5 @@ | |||
export interface StatsData { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its better to have a separate directory of all Type Aliases & Interfaces instead of keeping them inside a "constants" folder.
This issue is automatically marked stale because it hasn't received any recent activity. Hence, it'll be closed if no further activity occurs. |
Changes Include