-
Notifications
You must be signed in to change notification settings - Fork 115
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
Wen's movie site #51
base: main
Are you sure you want to change the base?
Wen's movie site #51
Conversation
…active class based on the page param
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.
Very well done! Neatly ordered and DRY code - we like 🥇 !
It feels like you thought about error handling and loading states throughout the code.
// Martin, Linda
@@ -2,9 +2,9 @@ | |||
<html lang="en"> | |||
<head> | |||
<meta charset="UTF-8" /> | |||
<link rel="icon" type="image/svg+xml" href="/vite.svg" /> | |||
<link rel="icon" type="image/svg+xml" href="/src/assets/cinema.png" /> |
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.
Nice Popcorn Icon 🍿 !
//Martin, Linda
<BrowserRouter> | ||
<Routes> | ||
<Route path="/" element={<Home />}> | ||
<Route index element={<Navigate replace to="browser" />} /> |
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.
Cool that you use navigate here. We didn't do that. Great inspiration!
// Martin, Linda
import styles from "../styling/Category.module.css"; | ||
import PropTypes from "prop-types"; | ||
|
||
const ListTypes = [ |
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.
Nice to use an array for the different endpoints.
// Martin, Linda
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.
Agree!
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.
But why PascalCase and not camelCase? 👀
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.
It's becuz the the variable is const and never gonna change. But maybe my impression was wrong, should we still use camelCase in this case @HIPPIEKICK ?
import { useEffect, useRef } from "react"; | ||
import PropTypes from "prop-types"; | ||
|
||
const LoadImage = ({ src, alt, className }) => { |
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.
Can we get a tutorial pls 😄 because we don't know how it works exactly.
// Martin, Linda
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.
Me too 😉
.finally(() => setIsLoading(false)); | ||
}, [searchParams]); | ||
|
||
const changePage = e => { |
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.
We would have liked to implement it too if we had time. Nice solution! 💯
//Martin, Linda
font-weight: 500; | ||
} | ||
|
||
.back:hover { |
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.
Elegant solution to use the gap property to animate the hover effect.
// Martin, Linda
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.
Well done Wen! Cool to see that you made this project your own. It looks super and has some really nice extra features. Be proud ✨
import styles from "../styling/Category.module.css"; | ||
import PropTypes from "prop-types"; | ||
|
||
const ListTypes = [ |
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.
Agree!
import styles from "../styling/Category.module.css"; | ||
import PropTypes from "prop-types"; | ||
|
||
const ListTypes = [ |
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.
But why PascalCase and not camelCase? 👀
className={({ isActive }) => | ||
isActive && category === listType.endpoint | ||
? `${styles.categoryLink} ${styles.active}` |
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.
Genius!
<Lottie | ||
className={styles.animation} | ||
animationData={color === "pink" ? errorPink : errorBlack} | ||
loop={false} | ||
/> |
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.
Very nice Lottie animations you've found!
import { useEffect, useRef } from "react"; | ||
import PropTypes from "prop-types"; | ||
|
||
const LoadImage = ({ src, alt, className }) => { |
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.
Me too 😉
src/components/LoadImage.jsx
Outdated
<> | ||
<img | ||
ref={imgRef} | ||
src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAAAXNSR0IArs4c6QAAAA1JREFUGFdjYGBg+A8AAQQBAHAgZQsAAAAASUVORK5CYII=" | ||
data-src={src} | ||
alt={alt} | ||
loading="lazy" | ||
className={className} | ||
/> | ||
</> |
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.
Very nice with lazy loading and that you found use for the useRef hook 🔥 Do you need the extra fragment though? 👀
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.
You're right I remove the extra fragment
src/components/Pagination.jsx
Outdated
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.
Very nice extra feature! ⭐
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.
And nicely solved
src/pages/PageNotFound.jsx
Outdated
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.
Another cool addition to your project, but make sure to fix the mobile view so that it's responsive too 😊
Remove loading="lazy" attribute to make it fully listen to the observer and remove the extra fragment.
Netlify link
https://popcorn-movie-hub.netlify.app/
Collaborators
[wwenzz]