-
Notifications
You must be signed in to change notification settings - Fork 0
feat: implement Main page #2
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
base: main
Are you sure you want to change the base?
Conversation
…t-app into feat/main-page
| "hooks": "@/hooks" | ||
| }, | ||
| "iconLibrary": "lucide" | ||
| } No newline at end of file |
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.
missing emptyline
|
|
||
| const LoginPage = () => { | ||
| const redirectToGitHub = () => { | ||
| const githubAuthUrl = "https://github.com/login/oauth/authorize"; |
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.
should be in .env
| import styles from "./LoginPage.module.css"; | ||
| import Image from "next/image"; | ||
|
|
||
| const GITHUB_CLIENT_ID = "YOUR_GITHUB_CLIENT_ID"; |
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.
should be in the env
| const redirectUri = encodeURIComponent( | ||
| "http://localhost:3000/auth/github/callback" | ||
| ); | ||
| const scope = "read:user user:email"; |
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 scope should be configurable
| icon: ( | ||
| <ThumbsUp | ||
| color="blue" | ||
| size={16} |
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.
(not critical)
I would prefer if we define the size of the icons as a const and then use in the list.
|
|
||
| const maxDonatorsShown = 21; | ||
|
|
||
| class FooterLayout extends React.Component<any, 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.
Why not a functional component?
| import { SocialNetworks } from "./SocialNetworks"; | ||
| import { Donation } from "./Donation"; | ||
|
|
||
| const maxDonatorsShown = 21; |
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.
(not crirical)
It is common practice to use ALL_CAPS for the constants
| const githubIssuesUrl = 'https://github.com/rolling-scopes/rsschool-app/issues'; | ||
| const publicRoutes = [ | ||
| { | ||
| icon: <Files color="#52c41a" size={16} />, |
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.
same for the size
| newTab: boolean; | ||
| }; | ||
|
|
||
| class Menu extends React.Component<MenuProps> { |
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.
should be a functinal compoenent
|
|
||
| const socialLinks = [ | ||
| { | ||
| icon: <GitHubOutlined size={24} />, |
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.
same for the icons
✅ Description
Main PageHeaderandFootercomponentsfavicon🖼️ Screenshots
🔧 To Do (Next Steps):
Say Thank you (Discord >> #gratitude)pageHeroespage for supporters