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

Router refactor #22

Merged
merged 9 commits into from
Dec 9, 2020
Merged

Router refactor #22

merged 9 commits into from
Dec 9, 2020

Conversation

demaceo
Copy link
Owner

@demaceo demaceo commented Dec 9, 2020

What does this PR do?

  • Add & reformat stylings for the NavBar component
  • Add movies to the FeaturedMovies carousel
  • Error Handling for SelectedMovie component

Where should the reviewer start?

App.js, FeatureMovies.js, SelectedMovie.js

How should this be manually tested?

Click on a MovieCard to be redirected to the SelectedMovie's page.

Any background context you want to provide?

What are the relevant tickets?

Closes #18 #17 #16 #15 #14 #8

@@ -7,60 +7,53 @@ import Catalogue from "./Components/Catalogue/Catalogue";
import { getMovies, getMovie } from "./apiCalls";
import "bootstrap/dist/css/bootstrap.min.css";
import "./App.scss";
import { Switch, Route, Link } from "react-router-dom";
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we are not using Switch, we should remove the reference

Comment on lines +43 to +47
<Row className="d-flex justify-content-center">
<Col md="auto">
{this.state.error && <Alert variant="danger">{this.state.error}</Alert>}
</Col>
</Row>
Copy link
Collaborator

Choose a reason for hiding this comment

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

love this error handling

key={movie.id}
handleClick={props.handleClick}
/>
<Link to={`/movie/${movie.id}`}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

we need to add a KEY peramiter to these!!!

Comment on lines -11 to -12
src="https://images.unsplash.com/photo-1485846234645-a62644f84728?ixid=MXwxMjA3fDB8MHxwaG90by1wYWdlfHx8fGVufDB8fHw%3D&ixlib=rb-1.2.1&auto=format&fit=crop&w=1940&q=80"
alt="Movie Clacker"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I enjoy the updates to this!

@@ -1,17 +1,12 @@
import React from "react";
import { Navbar, Nav, NavDropdown } from "react-bootstrap";
import "./NavBar.scss";
import { Link } from 'react-router-dom'
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can remove this I believe

@CalebCyphers CalebCyphers merged commit 8b58c45 into main Dec 9, 2020
@CalebCyphers CalebCyphers mentioned this pull request Dec 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Forward and back arrow navigation
2 participants