-
Notifications
You must be signed in to change notification settings - Fork 0
Two team compare #46
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: master
Are you sure you want to change the base?
Two team compare #46
Conversation
| reason: | ||
| "Compare Two Validation Error: Forms contain data from multiple different teams.", | ||
| }); | ||
| }), |
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.
is this really needed? find out if this is redundant and if not, consider type checking the query instead or maybe even moving to a body.
| import { spawn } from "child_process"; | ||
|
|
||
| const isDev = process.env.NODE_ENV === "DEV"; | ||
| const isDev = process.env.NODE_ENV !== "DEV"; |
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.
remove before merging
| const [firstTeam, secondTeam] = await Promise.all([ | ||
| fetchTeamCompareData(firstElement(selectedTeams)), | ||
| fetchTeamCompareData(secondElement(selectedTeams)), | ||
| ]); |
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.
consider just mapping the selected teams to fetchTeamCompare
| : "bg-rose-500/5 text-rose-500/60 border-rose-500/10"; | ||
| }; | ||
|
|
||
| const levelToScore = (level: 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.
consider using a keyof Levels or something apropriate instead of string. (idk the correct type here just use somoething narrower than string)
|
|
||
| export const compareRouter = Router(); | ||
|
|
||
| type GamePeriod = "auto" | "fullGame"; |
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.
What about autonomous? Consider adding it
|
|
||
| export const compareRouter = Router(); | ||
|
|
||
| type GamePeriod = "auto" | "fullGame"; |
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.
What about teleop? Consider adding it
|
|
||
| type GamePeriod = "auto" | "fullGame"; | ||
|
|
||
| const DIGITS_AFTER_DOT = 2; |
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.
Consider renaming it to 'DIFITS_AFTER_DECIMAL_DOT'
| const calculateAverageScoredFuel = ( | ||
| forms: ScoutingForm[], | ||
| gamePeriod: GamePeriod, | ||
| ) => { |
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.
Consider declaring a type 'L1 | L2 | L3 | L4' that the function will return
| : "L0"; | ||
| }; | ||
|
|
||
| const findTimesClimbedToMax = ( |
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.
Consider changing it to 'findTimesClimbedToMax'
| const fetchTeamCompareData = async (teamNumber: number) => { | ||
| const params = new URLSearchParams({ teamNumber: teamNumber.toString() }); | ||
| const url = `${compareUrl}?${params.toString()}`; | ||
|
|
||
| try { | ||
| const response = await fetch(url, { | ||
| method: "GET", | ||
| headers: { "Content-Type": "application/json" }, | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| const errorText = await response.text(); | ||
| throw new Error(`Server Error: ${errorText}`); | ||
| } | ||
|
|
||
| const data = await response.json(); | ||
| return data.teamCompareData as TeamCompareData; | ||
| } catch (err) { | ||
| console.error("Fetch failed:", err); | ||
| throw err; | ||
| } | ||
| }; |
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.
Consider moving this function to the backend, and call it in the frontend
| console.error("Fetch failed:", err); | ||
| throw err; | ||
| } | ||
| }; |
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 here
| const StatBox = ({ | ||
| label, | ||
| value, | ||
| color, | ||
| }: { |
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.
Consider declaring an interface statBoxProps
| </div> | ||
| ); | ||
|
|
||
| const LevelMiniStat = ({ label, count }: { label: string; count: number }) => ( |
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 here, consider adding props
| }; | ||
|
|
||
| const handleCompare = async () => { | ||
| if (selectedTeams.length !== MAX_SELECTED_TEAMS) return; |
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.
| if (selectedTeams.length !== MAX_SELECTED_TEAMS) return; | |
| if (selectedTeams.length <= MAX_SELECTED_TEAMS) return; |
compares between two teams