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

Danielle/535 add current entry number in header of entry page #615

Merged
merged 15 commits into from
Oct 25, 2024
Merged

Danielle/535 add current entry number in header of entry page #615

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
8d5e534
fix: replace h1 with Heading component
Danielle254 Oct 18, 2024
2036a63
feat: retrieve and save entry name if user is signed in
Danielle254 Oct 18, 2024
5d70e71
feat: add second Heading component with entry name
Danielle254 Oct 18, 2024
f569672
refactor: added indentation for Heading component props
Danielle254 Oct 18, 2024
d2ffa9e
Merge branch 'develop' of https://github.com/LetsGetTechnical/gridiro…
Danielle254 Oct 18, 2024
89459e3
refactor: replace with correct opening and closing tag syntax with ch…
Danielle254 Oct 18, 2024
c646f00
style: update color of h2 entry number to muted foreground on advise …
Danielle254 Oct 21, 2024
bfe5b50
fix: create test to check that new h2 element renders correct content…
Danielle254 Oct 21, 2024
1fd9e8e
fix: adjust entry name content - remove unneeded text "Entry X"
Danielle254 Oct 21, 2024
3d90bc5
fix: update testid to follow BEM naming conventions
Danielle254 Oct 21, 2024
20f6e29
fix: update testid to follow BEM naming conventions
Danielle254 Oct 21, 2024
efc93b0
fix: update expected text content of h2 entry name element
Danielle254 Oct 21, 2024
d8823c8
fix: test proper text content of new h1 week number element
Danielle254 Oct 21, 2024
9acaef7
Merge branch 'develop' into danielle/535-add-current-entry-number-in-…
Danielle254 Oct 23, 2024
abf45a7
Merge branch 'develop' into danielle/535-add-current-entry-number-in-…
Danielle254 Oct 24, 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
2 changes: 2 additions & 0 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 @@ -186,6 +186,8 @@ describe('League Week Picks', () => {
// Wait for the main content to be displayed
await waitFor(() => {
expect(screen.getByTestId('weekly-picks')).toBeInTheDocument();
expect(screen.getByTestId('week__week-number')).toHaveTextContent('Week 1');
expect(screen.getByTestId('week__entry-name')).toHaveTextContent('Entry 1');
choir241 marked this conversation as resolved.
Show resolved Hide resolved
});

expect(screen.queryByTestId('global-spinner')).not.toBeInTheDocument();
Expand Down
21 changes: 16 additions & 5 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 @@ -34,6 +34,7 @@ import Image from 'next/image';
import { useRouter } from 'next/navigation';
import LinkCustom from '@/components/LinkCustom/LinkCustom';
import { ChevronLeft } from 'lucide-react';
import Heading from '@/components/Heading/Heading';

/**
* Renders the weekly picks page.
Expand All @@ -43,6 +44,7 @@ import { ChevronLeft } from 'lucide-react';
// eslint-disable-next-line no-unused-vars
const Week = ({ entry, league, NFLTeams, week }: IWeekProps): JSX.Element => {
const [pickHistory, setPickHistory] = useState<string[]>([]);
const [entryName, setEntryName] = useState<string>('');
const [error, setError] = useState<string | null>(null);
const [schedule, setSchedule] = useState<ISchedule[]>([]);
const [selectedLeague, setSelectedLeague] = useState<ILeague | undefined>();
Expand Down Expand Up @@ -156,7 +158,8 @@ const Week = ({ entry, league, NFLTeams, week }: IWeekProps): JSX.Element => {
if (!currentEntry) {
throw new Error('Entry not found');
}


setEntryName(currentEntry.name);
let entryHistory = currentEntry?.selectedTeams || [];

if (currentEntry?.selectedTeams.length > 0) {
Expand Down Expand Up @@ -271,10 +274,18 @@ const Week = ({ entry, league, NFLTeams, week }: IWeekProps): JSX.Element => {
className="flex flex-col items-center w-full pt-8"
data-testid="weekly-picks"
>
<h1 className="pb-8 text-center text-[2rem] font-bold text-foreground">
Week {week} pick
</h1>

<Heading
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not test that H1 is on the screen and has the correct value as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ryandotfurrer How come all headings do not have pb-8 by default? Heading's usually have margin from the next element.

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 complete

as='h1'
className='pb-8'
data-testid='week__week-number'
>{`Week ${week} pick`}
</Heading>
<Heading
as='h2'
className='pb-8 text-muted-foreground'
data-testid='week__entry-name'
>{entryName}
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry if this is a dumb question, but is there a reason we're not just using currentEntry.name here instead of creating state for it? It never changes, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mhchen currenEntry.name is inside of a function call and thus not available to the greater Week component. That's why I saved that piece of data to state when the function is called. I was not 100% confident in this approach though so please feel free to dissect this!

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh nope, you’re right. Turns out it was a dumb question!

I’m so used to a different data fetching paradigm that I forgot this is necessary sometimes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Mike does bring up a good point here. This is not part of this ticket, but because we're now tracking 2 pieces of data from the response object, we should refactor the state to store the pick history and the entry name. A good ticket to refactor some code into the backlog.

</Heading>
{pickHistory.length > 0 && (
<section
className="flex flex-wrap w-[90%] gap-4 overflow-x-scroll justify-center pb-10 items-center"
Expand Down