feature(website): add books page #583
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
Visit the preview URL for this PR (updated for commit ffff677): https://si-admin-staging--pr583-pranav-migrate-books-kt3kl11t.web.app (expires Fri, 27 Oct 2023 15:02:06 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: b7b0969384059dce6ea8fad1ee1d1737e54e6676 |
There was a problem hiding this comment.
Very, very nice work @DarkMenacer 👏🏼👏🏼😊 I like it a lot how you separated the page into separate components.
I made a few comments where you can clean up the code a bit. Please let me know if you have any questions. And ping me, as soon as I should have another look.
website/src/components/book/book.tsx
Outdated
| <Typography as="div" className="mt-6 text-blue-500 sm:mb-2"> | ||
| {author} | ||
| </Typography> | ||
| {currentlyReading ? ( |
There was a problem hiding this comment.
nit: Instead of using ? here, you can do it with a && operator, which is a little cleaner:
{currentlyReading && (
<Typography
as="span"
size="xs"
weight="bold"
className="my-1 rounded-md bg-blue-500 px-2 py-0.5 text-white sm:mx-1"
>
Currently reading
</Typography>
)}
website/src/components/book/book.tsx
Outdated
|
|
||
| return ( | ||
| <BaseContainer> | ||
| <div className={baseClass}> |
There was a problem hiding this comment.
You can use the classNames() function here, e.g.:
className={classNames('flex h-fit flex-col flex-nowrap pb-5 shadow-md sm:flex-row', {
'my-40': currentlyReading,
'my-8': !currentlyReading,
})}
website/src/components/book/book.tsx
Outdated
| {author} | ||
| </Typography> | ||
| {currentlyReading ? ( | ||
| <Typography |
There was a problem hiding this comment.
Instead of using the Typography component here, you could use the Badge component from the ui. See: https://ui.shadcn.com/docs/components/badge
website/src/components/book/book.tsx
Outdated
| {quote} | ||
| </Typography> | ||
| <Typography as="div" weight="bold"> | ||
| <a href={publisherLink} className="hover:text-blue-800"> |
There was a problem hiding this comment.
Same as above: use the Link component and wrap the Typography inside the Link component.
website/src/components/book/book.tsx
Outdated
| </div> | ||
| <div className="mx-8 flex w-fit basis-5/12 flex-col"> | ||
| <div className="flex flex-col items-baseline sm:flex-row"> | ||
| <Typography as="div" className="mt-6 text-blue-500 sm:mb-2"> |
There was a problem hiding this comment.
as="div" is not needed. You can generally remove the parameter where you use it in this file.
website/src/components/book/book.tsx
Outdated
| <div className="w-fit basis-1/3"> | ||
| <Image src={cover} height={4000} alt="book cover" /> | ||
| </div> | ||
| <div className="mx-8 flex w-fit basis-5/12 flex-col"> |
There was a problem hiding this comment.
Why do you use basis-5/12 here and not basis-2/3 when you use basis-1/3 above?
There was a problem hiding this comment.
I think I took that decision looking at how the previous website was made and basis-2/3 might have been a bit too wide.
| return ( | ||
| <BaseContainer className="mt-20 flex flex-col content-center justify-center text-center"> | ||
| <Typography as="div" size="4xl" weight="bold" lineHeight="loose" className="m-3 w-auto pb-4"> | ||
| Recommended Reading |
There was a problem hiding this comment.
Can you put these translations also into the website-books.json file and use the Translator as in section-2.tsx?
website/src/components/book/book.tsx
Outdated
| )} | ||
| </div> | ||
| <Typography as="div" weight="bold" size="xl" lineHeight="relaxed" className="mb-1"> | ||
| <a href={authorLink}>{title}</a> |
There was a problem hiding this comment.
Please use the Link component from Next here. Also, wrap the Typography inside the Link component.
No description provided.