Alex/Add labels for home and away #521
Conversation
|
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.
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| @@ -59,25 +59,34 @@ const WeekTeams = ({ | |||
| > | |||
| {schedule.map((scheduledGame) => ( | |||
| <div | |||
| className="grid w-full grid-cols-2 gap-4 pb-8" | |||
| className="grid w-full grid-cols-[1fr_auto_1fr] gap-4 pb-8" | |||
| style={{ direction: 'rtl' }} | |||
There was a problem hiding this comment.
Why is this here? This is typically used to support languages that are written right to left, unlike english which is written left to right.
There was a problem hiding this comment.
it was shashi's suggestion. he wanted to flip everything in the grid layout so that the away teams were on the left and home teams were on right. because the data always rendered vise versa
There was a problem hiding this comment.
This is correct. The data receive from ESPN is reversed. We don't alter the source data, we have to work with it.
| <> | ||
| {index > 0 && ( | ||
| <div className="h-20 flex self-end items-center"> | ||
| <span className="mx-2">@</span> |
There was a problem hiding this comment.
this probably does not need additional margin
| @@ -7,6 +7,7 @@ import { Label } from '../Label/Label'; | |||
| import { RadioGroupItem } from '../RadioGroup/RadioGroup'; | |||
|
|
|||
| type WeeklyPickButtonProps = { | |||
| homeAway: string; | |||
There was a problem hiding this comment.
nit: I feel like this is not a very clear prop name, and, while this is a p1 fix, maybe we should revisit it in the future.
There was a problem hiding this comment.
its already being prop drilled from another component as "competition.homeAway" and homeAway is a prop from an interface [] with 20 other props lol. but the actual prop itself is just a string.
There was a problem hiding this comment.
So obviously leave here as is. I wonder if this is worth putting as a ticket. @shashilo, thoughts?
| @@ -15,10 +16,14 @@ describe('WeeklyPickButton', () => { | |||
| it('renders correctly', () => { | |||
| render( | |||
| <RadioGroup> | |||
| <WeeklyPickButton team={weeklyPickData.team} src={weeklyPickData.src} /> | |||
| <WeeklyPickButton homeAway={weeklyPickData.homeAway} team={weeklyPickData.team} src={weeklyPickData.src} /> | |||
There was a problem hiding this comment.
Need to write 2 separate tests for this. 1 to check that the team is home and 1 that checks the team is away.
| @@ -17,6 +17,7 @@ const meta = { | |||
| }, | |||
| }, | |||
| argTypes: { | |||
| homeAway: { description: 'Location of game' }, | |||
There was a problem hiding this comment.
Are we adding locations of the games to the data in a separate ticket?
There was a problem hiding this comment.
I had to add this to make the build pass. Not sure on it tho. @shashilo thoughts?
| <div style={{ direction: 'ltr' }} className="flex items-center mt-4"> | ||
| <RadioGroupItem | ||
| value={team} | ||
| id={team} |
There was a problem hiding this comment.
Is this the teamName, teamLogo, teamId? A bit ambiguous/unclear variable naming here.
There was a problem hiding this comment.
i didn't write this. it was copy and pasted to add my changes
There was a problem hiding this comment.
What do you mean by copy and pasted, like merge conflicts?
There was a problem hiding this comment.
i actually copied and pasted it. when i add in wrappers... in this case the <></> wrapper, i copy everything, delete, add the wrapper, then paste everything back inside. I see now I need to be careful with it to avoid confusion. The reason why I added the <></> is because i had to add {homeAway} on top and everything has to be inside the same parent element so i added the empty fragments
Closes #520
Notes: