-
Notifications
You must be signed in to change notification settings - Fork 14
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
request a code review #3
base: main
Are you sure you want to change the base?
Conversation
aplicacion vitejs
feat: i do typescript type
feat: I add formatMovie function and unit testing
Fix: unit testing and formatMovie
continudad del H U 2
feat: MovieList
feat: add Home component
feat> add css to moviecard, movielist and app
feat: add more style
feat: add unit testing for MovieCard
feat: add unit testing for MovieList
feat: add update of get movies to pagination
feat: i start pagination component
feat: finish pagination component
feat: add getMovieGenres
feat: add a unit testing for paginanation component
feat: add formatGenresToMap
fix: filter by
feat: i start movieDetail component
feat: add the last component (movie detail)
fix: unit testing
fix: unit testing
fix: movie detail test
feat: (readme)i change readme
Update README.md
<html lang="en"> | ||
<head> | ||
<meta charset="UTF-8" /> | ||
<link rel="icon" type="image/svg+xml" href="/vite.svg" /> |
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.
change the default icon
}, | ||
"dependencies": { | ||
"dotenv": "^16.4.5", | ||
"jsdom": "^24.1.0", |
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 is a dependence of develp
<link rel="preconnect" href="https://fonts.googleapis.com" /> | ||
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin /> | ||
<link | ||
href="https://fonts.googleapis.com/css2?family=Lexend:[email protected]&display=swap" | ||
rel="stylesheet" | ||
/> |
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.
Maybe you can move the fonts to style.css
<style> @import url('https://fonts.googleapis.com/css2?family=Roboto:ital,wght@0,100;0,300;0,400;0,500;0,700;0,900;1,100;1,300;1,400;1,500;1,700;1,900&display=swap'); </style>"react-dom": "^18.2.0", | ||
"react-loading": "^2.0.3", | ||
"react-router-dom": "^6.23.0", | ||
"vitest": "^1.6.0" |
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.
tienes 2 entornos de test
import Home from "./components/Home"; | ||
import { MovieDetail } from "./components/MovieDetail"; |
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.
import Home from "./components/Home"; | |
import { MovieDetail } from "./components/MovieDetail"; | |
import Home from "./components/Home"; | |
import MovieDetail from "./components/MovieDetail"; |
consistency
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.
All components with export default
]; | ||
|
||
setTimeout(() => { | ||
expect(getByText(moviesExpected[0].title)).toBeInTheDocument(); |
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.
Maybe try snapshot
id="seletFilter" | ||
value={selectedOption?.value || ""} | ||
onChange={(event) => { | ||
const selectedOption = options.find(option => option.value === event.target.value); |
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.
useMemo
@@ -0,0 +1,10 @@ | |||
import React from 'react' |
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 the future try install eslint and prettier
export function getMovieGenres(): Promise<[{ id: number; name: string }]> { | ||
return fetch(`https://api.themoviedb.org/3/genre/movie/list?language=en`, { | ||
method: "GET", | ||
headers: { | ||
accept: "application/json", | ||
Authorization: `Bearer ${token}`, | ||
}, | ||
}) | ||
.then((response) => response.json()) | ||
.then((genresResult) => { | ||
return genresResult.genres; | ||
}) | ||
.catch((error) => { | ||
throw error; | ||
}); |
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.
async/ await
Update README.md
No description provided.