Skip to content
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.

Sign up for GitHub

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

#548 P0: Chris/fix previous picks #549

Merged
merged 5 commits into from
Sep 20, 2024
Merged

#548 P0: Chris/fix previous picks #549

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fdaae4f
initial fix without tests
chris-nowicki Sep 20, 2024
42edf0f
fix test for no pick
chris-nowicki Sep 20, 2024
b732f61
fix test for no pick on league entry card
chris-nowicki Sep 20, 2024
dff9b78
fix jennifer comments
chris-nowicki Sep 20, 2024
cea643e
Merge branch 'develop' into chris/fix-previous-picks
shashilo Sep 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 46 additions & 12 deletions app/(main)/league/[leagueId]/entry/[entryId]/week/Week.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,12 +184,9 @@ describe('League Week Picks', () => {
);

// Wait for the main content to be displayed
await waitFor(
() => {
expect(screen.getByTestId('weekly-picks')).toBeInTheDocument();
},
{ timeout: 5000 },
);
await waitFor(() => {
expect(screen.getByTestId('weekly-picks')).toBeInTheDocument();
});

expect(screen.queryByTestId('global-spinner')).not.toBeInTheDocument();
});
Expand Down Expand Up @@ -241,12 +238,9 @@ describe('League Week Picks', () => {
/>,
);

await waitFor(
() => {
expect(screen.getByTestId('weekly-picks')).toBeInTheDocument();
},
{ timeout: 5000 },
);
await waitFor(() => {
expect(screen.getByTestId('weekly-picks')).toBeInTheDocument();
});

expect(screen.getByTestId('user-pick-history')).toBeInTheDocument();

Expand All @@ -261,6 +255,46 @@ describe('League Week Picks', () => {
'/_next/image?url=https%3A%2F%2Fexample.com%2Fbrowns.png&w=128&q=75',
);
});
it('should show previous week pick as no pick if there is no history of selected teams', async () => {
mockUseAuthContext.isSignedIn = true;
const mockWeek = '2';
(getCurrentUserEntries as jest.Mock).mockResolvedValue([
{
$id: '123',
name: 'Entry 1',
user: '123',
league: '123',
selectedTeams: ['', 'Browns'],
eliminated: false,
},
]);

render(
<Week
entry={entry}
league={league}
NFLTeams={NFLTeams}
week={mockWeek}
/>,
);

await waitFor(() => {
expect(screen.getByTestId('weekly-picks')).toBeInTheDocument();
});

expect(screen.getByTestId('user-pick-history')).toBeInTheDocument();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I know there was a conversation on your other PR about declaring variable vs inline but I think this scenario where you have to write screen.getByTestId('user-pick-history') twice is a good use case to save it in a variable instead to reuse.

Both approaches work though! The code still runs but might save you or someone else in the future sometime. Of course, we'll see what Shashi thinks because I'm curious which is better or how much it matters😅.

Copy link
Collaborator

@shashilo shashilo Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When there's more than 2, that's always a good case to make a variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shashilo @jennifertieu thanks! yes, moving forward I will be doing this when there are more than 2.

expect(screen.getByTestId('user-pick-history')).toHaveTextContent(
'No Pick',
);

expect(screen.getByTestId('no-pick')).toBeInTheDocument();
const userPickHistoryLogos = screen.queryAllByTestId('league-history-logo');
expect(userPickHistoryLogos).toHaveLength(1);
expect(userPickHistoryLogos[0]).toHaveAttribute(
'src',
'/_next/image?url=https%3A%2F%2Fexample.com%2Fbrowns.png&w=128&q=75',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: Why is the URL string encoded like this instead of url=https://example.com/browns.png? (same questions with the other URLs)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jennifertieu great question! the component is using Next Image. This is how Next Image formats the url for images so it can apply its customizations.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chris-nowicki replying late lol but that is good to know!! I'll keep that in mind when testing with Next in the future, thanks Chris!

);
});

xit('should show success notification after changing your team pick', async () => {
(createWeeklyPicks as jest.Mock).mockResolvedValue({});
Expand Down
27 changes: 18 additions & 9 deletions app/(main)/league/[leagueId]/entry/[entryId]/week/Week.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ const Week = ({ entry, league, NFLTeams, week }: IWeekProps): JSX.Element => {

return (
<div
key={`${logoURL}-${index + 1}`}
key={`${logoURL ? logoURL : 'no-pick'}-${index + 1}`}
className={cn(
'flex flex-col items-center justify-center border p-2 rounded-lg gap-1',
isCurrentWeek && hasCurrentWeekPick
Expand All @@ -298,14 +298,23 @@ const Week = ({ entry, league, NFLTeams, week }: IWeekProps): JSX.Element => {
? 'CURRENT'
: `WEEK ${index + 1}`}
</span>
<Image
className="league-entry-logo"
width={64}
height={64}
data-testid="league-history-logo"
src={logoURL}
alt="teamLogo"
/>
{logoURL ? (
<Image
className="league-entry-logo"
width={64}
height={64}
data-testid="league-history-logo"
src={logoURL}
alt="teamLogo"
/>
) : (
<span
className="text-xs h-16 w-16 text-primary pt-6 text-center"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried about the a11y of the color and the small text size, did you check this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryandotfurrer I did not check this. hmmmmm @shashilo what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine for now. A11Y audit can come after.

data-testid="no-pick"
>
No Pick
</span>
)}
</div>
);
})}
Expand Down
29 changes: 29 additions & 0 deletions components/LeagueEntries/LeagueEntries.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,4 +113,33 @@ describe('LeagueEntries', () => {
'/_next/image?url=%2Fteam-a-logo.png&w=96&q=75',
);
});
it('renders no pick when a previous week pick is not available', () => {
const teamLogoUrl = 'https://example.com/logo.png';
const linkUrl = '/change-pick';

render(
<LeagueEntries
entryName="Entry 2"
isPickSet={true}
linkUrl={linkUrl}
selectedTeamLogo={teamLogoUrl}
userPickHistory={['', '/team-a-logo.png']}
/>,
);

const leagueEntryLogo = screen.getByTestId('league-entry-logo');
const noPick = screen.getByTestId('no-pick');

expect(leagueEntryLogo).toBeInTheDocument();
expect(leagueEntryLogo).toHaveAttribute(
'src',
'/_next/image?url=https%3A%2F%2Fexample.com%2Flogo.png&w=96&q=75',
);
expect(screen.getByTestId('league-history-logo')).toHaveAttribute(
'src',
'/_next/image?url=%2Fteam-a-logo.png&w=96&q=75',
);
expect(noPick).toBeInTheDocument();
expect(noPick).toHaveTextContent('No Pick');
});
});
27 changes: 18 additions & 9 deletions components/LeagueEntries/LeagueEntries.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,18 +59,27 @@ const LeagueEntries = ({
>
{userPickHistory?.map((logoURL, index) => (
<div
key={logoURL}
key={`${logoURL ? logoURL : 'no-pick'}-${index + 1}`}
className="flex flex-col h-[66px] opacity-80 items-center justify-center border border-border px-4 py-1 rounded-lg gap-1"
>
<span className="text-xs">WEEK {index + 1}</span>
<Image
className="league-entry-logo"
width={40}
height={40}
data-testid="league-history-logo"
src={logoURL}
alt="teamLogo"
/>
{logoURL ? (
<Image
className="league-entry-logo"
width={40}
height={40}
data-testid="league-history-logo"
src={logoURL}
alt="teamLogo"
/>
) : (
<span
className="text-xs h-10 w-full text-primary pt-2"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm worried about the a11y of the color and the small text size, did you check this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryandotfurrer I did not check this. hmmmmm @shashilo what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine for now. A11Y audit can come after.

data-testid="no-pick"
>
No Pick
</span>
)}
</div>
))}
</section>
Expand Down