-
Notifications
You must be signed in to change notification settings - Fork 1
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
ui/ux: update week page #370
Conversation
…nt leaguename for breadcrumb link
… and altered linkcustom component LinkCustom component accepted a prop of `children` that previsusly only accepted a type of `string`. I've updated this accept a type of React.ReactNode so passing icons as children would not throw an error.
…> radiogroup removed mb-4 from FormItem
… submit so it would send it's data to the database
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Gridiron Survivor Application
Project name: Gridiron Survivor Application
Only deployments on the production branch are activated automatically. If you'd like to activate this deployment, navigate to your deployments. Learn more about Appwrite Function deployments.
|
@@ -28,12 +31,22 @@ import { ISchedule } from './WeekTeams.interface'; | |||
// eslint-disable-next-line no-unused-vars | |||
const Week = ({ league, NFLTeams, week }: IWeekProps): JSX.Element => { | |||
const [schedule, setSchedule] = useState<ISchedule[]>([]); | |||
const [selectedLeague, setSelectedLeague] = useState<ILeague | undefined>(); |
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 the state be initialized to string? i.e.: const [selectedLeague, setSelectedLeague] = useState<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.
I'm gonna leave this to @chris-nowicki, since he helped me with this by implementing it Though, initializing it to a string makes sense to me!
<nav className="py-6 text-orange-500 hover:no-underline"> | ||
<LinkCustom | ||
className="text-orange-500 flex gap-3 items-center font-semibold text-xl hover:no-underline" | ||
href="/league/all" |
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.
Does the hover:bg-zinc-800 transition
from the WeeklyPickButton apply to this still when hovering over it on WeekTeams?
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.
IIRC it did in my testing, but now you're making me question myself! I'll confirm tomorrow (7/9/24) and report back.
…ge and components made it so the user tapping or clicking on their team selection updates the database
getSchedule(week); | ||
}, [week]); | ||
setIsLoading(false); | ||
}, [week, selectedLeague]); |
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.
By adding selectedLeague
, it's going to cause side effects. Every time selectedLeague() is called, it will run this useEffect.
@@ -28,12 +31,22 @@ import WeekTeams from './WeekTeams'; | |||
// eslint-disable-next-line no-unused-vars | |||
const Week = ({ entry, league, NFLTeams, week }: IWeekProps): JSX.Element => { | |||
const [schedule, setSchedule] = useState<ISchedule[]>([]); | |||
const [selectedLeague, setSelectedLeague] = useState<ILeague | undefined>(); | |||
const [isLoading, setIsLoading] = useState<boolean>(true); |
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.
nit: We'll need to look into Suspense in the near future.
<form | ||
className="mx-auto flex w-[90%] max-w-3xl flex-col items-center gap-8" | ||
onSubmit={form.handleSubmit(onSubmit)} | ||
<div> |
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.
No to classess Div's. This should have a class that matches the component name.
* @param dateStr The date string to format. | ||
* @returns The formatted date and time string. | ||
*/ | ||
const formatDateTime = (dateStr: string): 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.
Do not shorthand your variable names. When someone else is reading them, you want them to concise and ready to browse.
const formatDateTime = (dateStr: string): string => { | |
const formatDateTime = (dateString: string): string => { |
}: IWeekTeamsProps): JSX.Element => ( | ||
<> | ||
{schedule.map((scheduledGame) => ( | ||
<RadioGroup | ||
onValueChange={field.onChange} | ||
defaultValue={userPick} | ||
key={scheduledGame.id} | ||
className="grid w-full grid-cols-2 gap-4" | ||
className="grid w-full grid-cols-2 gap-4 pb-8" | ||
onChange={onWeeklyPickChange as FormEventHandler<HTMLDivElement>} |
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.
No go on assertions. Unless it's a special case, it's most likely your typing that is off. In this case, We need to update IWeekTeamsProps with the correct typing. We used the generic object
to get by testing.
Closes #353