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.

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

Open
wants to merge 59 commits into
base: main
Choose a base branch
from
Open

request a code review #3

wants to merge 59 commits into from

Conversation

Deniisolo
Copy link

No description provided.

Deniisolo and others added 30 commits April 24, 2024 14:07
feat: i do typescript type
feat: I add formatMovie function and unit testing
Fix: unit testing and formatMovie
feat> add css to moviecard, movielist and app
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
Deniisolo and others added 27 commits May 15, 2024 15:30
feat: finish  pagination component
feat: add a unit testing for paginanation component
feat: add formatGenresToMap
feat: i start movieDetail component
feat: add  the  last component (movie detail)
feat: (readme)i change readme
<html lang="en">
<head>
<meta charset="UTF-8" />
<link rel="icon" type="image/svg+xml" href="/vite.svg" />

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",

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

Comment on lines +7 to +12
<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"
/>

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"

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

Comment on lines +3 to +4
import Home from "./components/Home";
import { MovieDetail } from "./components/MovieDetail";

Choose a reason for hiding this comment

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

Suggested change
import Home from "./components/Home";
import { MovieDetail } from "./components/MovieDetail";
import Home from "./components/Home";
import MovieDetail from "./components/MovieDetail";

consistency

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();

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);

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'

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

Comment on lines +6 to +20
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;
});

Choose a reason for hiding this comment

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

async/ await

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants