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

movie-project-v3 #6

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

movie-project-v3 #6

wants to merge 16 commits into from

Conversation

hewr-srood
Copy link

@hewr-srood hewr-srood commented Jun 27, 2020

The movie project with all requirements of v-3
Team- Members:

Copy link
Contributor

@osamaakb osamaakb left a comment

Choose a reason for hiding this comment

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

good job guys, I really like your practices compared to the time you practiced react, check out the comments and keep up the good work

animation: scale-up-center 0.4s cubic-bezier(0.39, 0.575, 0.565, 1) both;
}

@-webkit-keyframes scale-up-center {
Copy link
Contributor

Choose a reason for hiding this comment

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

you could have used any css animation libraries for react like
https://www.npmjs.com/package/react-animated-css

import "bootstrap/dist/css/bootstrap.min.css";
import Myfooter from "./components/footer";
import Main from "./components/main";
let TMDB_BASE_URL = "https://api.themoviedb.org/3";
Copy link
Contributor

Choose a reason for hiding this comment

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

should be const

import Main from "./components/main";
let TMDB_BASE_URL = "https://api.themoviedb.org/3";

const constructUrl = (path, query) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would put this inside API.js file and add all network requests there and related network work there too

import React, { useState } from "react";
import { Card, Button, Badge } from "react-bootstrap";
import MovieOverview from './MovieOverview'
let imageBaseUrl = "https://image.tmdb.org/t/p/w500";
Copy link
Contributor

Choose a reason for hiding this comment

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

const here too

return (
<li className='m-5 tilt-in-top-1'>
<Card
style={{ width: "18rem", border: 'none' }}
Copy link
Contributor

Choose a reason for hiding this comment

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

try to avoid using inline styles

handleMovies={props.handleMovies}
handleGenresList={props.handleGenresList}
isLoaded={props.isLoaded}
setIsLoaded={props.setIsLoaded}
Copy link
Contributor

Choose a reason for hiding this comment

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

here too

<SearchBox
handleMovies={props.handleMovies}
handleQuery={props.handleQuery}
constructUrl={props.constructUrl}
Copy link
Contributor

Choose a reason for hiding this comment

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

why are you passing this here?

handleMovies={props.handleMovies}
handleQuery={props.handleQuery}
constructUrl={props.constructUrl}
query={props.query}
Copy link
Contributor

Choose a reason for hiding this comment

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

since you are passing the handleQuery here you don't need to pass query

.then(response => response.json())
.then(json => {
props.handleMovies(json.results);
element.value = "";
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you can remove the element and replace this one here with

Suggested change
element.value = "";
e.target.value = "";

import React from "react";
import { Spinner } from "react-bootstrap";

export default function MySpinner(props) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export default function MySpinner(props) {
export default function MySpinner({ show }) {

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.

3 participants