-
Notifications
You must be signed in to change notification settings - Fork 7
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
Feature/like button code conventions #14
Conversation
src/components/Footer/Footer.tsx
Outdated
@@ -38,6 +38,7 @@ const Footer: FC = () => { | |||
] | |||
|
|||
const { t } = useTranslation() | |||
const FOOTER_SUPPORT = "FOOTER.SUPPORT" |
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 don't understand why this is done
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 think it's pointless too, but eslint was making a big deal about re-using the same "FOOTER.SUPPORT" multiple times. Said to assign it to a variable and use that instead. Threw an error because of it. I can undo it or add a line for eslint to by pass it.
src/pages/coaching.tsx
Outdated
) | ||
|
||
bookmarkSearch === 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.
This should be unnecessary. The returned weeks array contains objects already have the value whether they've bookmarked or not
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.
Also, you're defining the bookmarked value outside the map, which means that it will always reassign as the loop runs. It should also be constant like const bookmarked = bookmarkeSearch ? true : false, instead of reassigned variable. But just remove this because it should not be needed.
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.
In your opinion, what's the best approach to show the bookmarked content on the cards (week/lesson). We have two data sets. The weeks
array and the bookmarkedData
array. I took the inner loop away, so it's just the weeks array getting mapped. I'm just wondering what you think is the best way.
<Weeks>
{weeks.map((week: ContentfulWeek) => {
return (
<WeekCard
bookmarked={false}
key={`${week?.slug}`}
path={`/week/${week?.slug}`}
intro={week?.intro}
name={week?.weekName}
duration={week?.duration}
lessons={week?.lessons}
coverPhoto={week?.coverPhoto?.fluid as FluidObject}
slug={week.slug}
/>
)
})}
</Weeks>
Description
Provide new bookmarking functionality for lessons, weeks and habits. Based on work made by Kayla on branch #6. I have refactored the data fetching logic so that it's easier to understand, and fixed type errors and race conditions.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist: