-
Notifications
You must be signed in to change notification settings - Fork 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
kevinsolorio/tg-14-create-the-choose-your-state-and-plot-status-pages #6
kevinsolorio/tg-14-create-the-choose-your-state-and-plot-status-pages #6
Conversation
<h1>Plot Status</h1> | ||
<h3>Do you own a plot to plant in?</h3> | ||
<ButtonContainer> | ||
<Button onClick={() => handleButtonClick('Yes')}>Georgia</Button> |
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 Yes and Georgia are flipped. Should be ('Georgia')}>Yes
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.
good point! I think it should be
<Button onClick={() => handleButtonClick('Yes')}>Yes</Button>
and the line below:
<Button onClick={() => handleButtonClick('No')}>No</Button>
Perhaps one way to reduce these types of errors is to create a list of possible options and map them into button components, so there's 1 source of truth for each button option
@@ -0,0 +1,31 @@ | |||
'use client'; |
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.
let's delete the app/onboarding/page.tsx
file!
export interface Profile { | ||
state: string; | ||
email: string; | ||
phone_num: string; |
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.
quick note: we may remove phone_num in the future (gonna discuss w rob on fri), but we can leave it for now.
@@ -2,6 +2,13 @@ import type { UUID } from 'crypto'; | |||
|
|||
export type Season = 'SPRING' | 'SUMMER' | 'FALL' | 'WINTER'; | |||
|
|||
export interface Profile { |
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.
we also want to include user_id: UUID
let's reference the notion as the source of truth for schema!
const { addProfile } = useProfile(); | ||
|
||
const handleButtonClick = (state: string) => { | ||
const newProfile = { state, email: '', phone_num: '', user_type: '' }; |
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.
it might be cleaner to type this as Partial, i.e.
const newProfile: Partial<Profile> = { state };
For more info on the Partial type: https://dev.to/martinpersson/better-code-quality-with-typescripts-utility-types-pick-partial-and-omit-3605
the creation of a new profile should be done outside the handleButtonClick function, since we want to udpate the same profile on each button click
import { useState } from 'react'; | ||
import { Profile } from '../types/schema'; | ||
|
||
const initialProfiles: Profile[] = []; |
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.
ooo this is interesting for testing! we would ultimately replace this with pushing the profiles directly to Supabase though.
if we're including it here, do you want to actually use it to test the functionality of your components?
in either case, before we merge this, we probably want to remove or comment out code used for testing
export default function Page() { | ||
const { addProfile } = useProfile(); | ||
|
||
const handleButtonClick = (state: string) => { |
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.
let's make sure to update this to handle the plot status!
and use the Partial<Profile>
type for the line below
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.
looking good so far! please see my comments; some of the comments on the plotstatus page apply to the state page and vice versa.
- use
Partial<Profile>
to define and update profiles - for now, define a new profile at the top of each component, instead of inside the handleButtonClick function
- consider mapping the button options so that there's a single source of truth
const handleButtonClick = (state: string) => { | ||
const newProfile = { state, email: '', phone_num: '', user_type: '' }; | ||
addProfile(newProfile); | ||
console.log('test'); |
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.
Maybe we can console.log(newProfile)
instead!
What's new in this PR 🧑🌾
Description
added the plot and state pages for onboarding, a profile management, and style folder with colors
Screenshots
How to review
check out new onboarding folder and the pgaes/styling. Run app and go to /onboarding/plotstatus or /onboarding/state to checkout
Next steps
Working on the profile context still, for this pr I just made an array that should store a profile information.
Relevant links
Online sources
Related PRs
Next PR should expand on this one
CC: @ccatherinetan