-
Notifications
You must be signed in to change notification settings - Fork 10
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
Login page #9
Login page #9
Conversation
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.
Reviewable status: 0 of 28 files reviewed, 20 unresolved discussions (waiting on @SKairinos)
src/pages/home/Home.tsx
line 19 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
remove this sample text and add some brief text welcoming CFL contributor's our contributor service and encouraging them to become a contributor.
Done.
src/components/GitHubLoginButton.tsx
line 8 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
Do not need to be an independent, reusable component since it will only ever be rendered on the login page. Define this component inline in the login page and delete this file.
Done.
src/components/GitHubLoginButton.tsx
line 13 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
sx = {theme => ({...})}
Done.
src/components/GitHubLoginButton.tsx
line 16 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
don't use absolute values. use relative values.
`${theme.spacing(4)} {theme.spacing(5)}`
Done.
src/components/GitHubLoginButton.tsx
line 18 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
try "black" or "black.main". if that doesn't work, try theme.palette.black.main
Done.
src/components/GitHubLoginButton.tsx
line 19 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
try
"white"
or"white.main"
. if that doesn't work, trytheme.palette.white.main
Done.
src/components/GitHubLoginButton.tsx
line 20 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
why? doesn't seem necessary.
Done.
src/components/GitHubLoginButton.tsx
line 21 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
why? doesn't seem necessary.
Done.
src/components/GitHubLoginButton.tsx
line 24 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
Use this icon, not an image.
Done.
src/images/github-mark-white.png
line 0 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
delete this file. use the icon that's already installed.
Done.
src/pages/login/GitHub.tsx
line 0 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
delete this file and merge it's contents into the home page. the home will be our login page.
Done.
src/pages/login/GitHub.tsx
line 14 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
not like this. we have a hook which helps you do this.
import { useSearchParams } from "codeforlife/hooks/router" import * as yup from "yup" ... const searchParams = useSearchParams({ code: yup.string() }) ... if (searchParams?.code) { // DO STUFF }
Done.
src/pages/login/GitHub.tsx
line 19 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
you must use the
useNavigate
hook to navigate between pages. you can only navigate after a component has mounted.import { useNavigate } from "codeforlife/hooks/router" ... const navigate = useNavigate() ... useEffect(() => { // navigate to the same page with "." navigate(".", { replace: true }) }, [...])
Done.
src/pages/login/GitHub.tsx
line 24 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
navigate(paths.agreementSignatures._)
Done.
src/pages/login/GitHub.tsx
line 27 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
You are already on the login page - there's no point in this.
Done.
src/pages/login/GitHub.tsx
line 36 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
Delete this div. Use MUI's Stack instead. You can add spacing between element's using the Stack's spacing property.
Done.
src/pages/login/GitHub.tsx
line 46 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
Define button inline.
Done.
src/routes/login.tsx
line 0 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
delete this file
Done.
src/routes/login.tsx
line 8 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
<Route path={paths._} element={<Home />} />
Done.
src/routes/paths.ts
line 10 at r1 (raw file):
Previously, SKairinos (Stefan Kairinos) wrote…
delete this.
Done.
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.
Reviewed 13 of 23 files at r2, all commit messages.
Reviewable status: 13 of 28 files reviewed, 1 unresolved discussion
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.
Reviewed 5 of 27 files at r1, 10 of 23 files at r2.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @SalmanAsh)
.vscode/launch.json
line 21 at r2 (raw file):
}, { "env": {
delete this
src/api/session.ts
line 10 at r2 (raw file):
const sessionApi = api.injectEndpoints({ endpoints: build => ({ loginWithGitHub: build.mutation<LoginWithGitHubResult, LoginWithGitHubArg>({
just login
src/app/env.ts
line 5 at r2 (raw file):
export * from "codeforlife/env" export const GH_CLIENT_ID = env.GITHUB_CLIENT_ID ?? "REPLACE_ME"
- make the entire link an env var
- add the env var in the .env file
- variable names must me the same
- env vars must start with "VITE_"
src/pages/home/Home.tsx
line 1 at r2 (raw file):
import * as pages from "codeforlife/components/page"
fix imports
src/pages/home/Home.tsx
line 34 at r2 (raw file):
navigate(paths.agreementSignatures._) } else if (code) { navigate(".", { replace: true })
move this into the catch below.
src/pages/home/Home.tsx
line 53 at r2 (raw file):
<Stack spacing={10} sx={{
us stack-level props
src/pages/home/Home.tsx
line 73 at r2 (raw file):
to={`https://github.com/login/oauth/authorize?client_id=${GH_CLIENT_ID}`} sx={theme => ({ marginTop: `${theme.spacing(20)}`,
no need for the string here
src/pages/home/Home.tsx
line 74 at r2 (raw file):
sx={theme => ({ marginTop: `${theme.spacing(20)}`, borderRadius: `${theme.spacing(1)}`,
no need for the string here
src/pages/home/Home.tsx
line 76 at r2 (raw file):
borderRadius: `${theme.spacing(1)}`, padding: `${theme.spacing(4)} ${theme.spacing(5)}`, fontSize: `${theme.spacing(2.5)}`,
no need for the string here
# Service. VITE_SERVICE_NAME=contributor
add VITE_LINK_GH_LOGIN
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.
Reviewed 16 of 16 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SalmanAsh)
🎉 This PR is included in version 1.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This change is