-
Notifications
You must be signed in to change notification settings - Fork 417
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
W8-movies-Ylva-Karlsson-and-Sandra-Olsson #318
base: master
Are you sure you want to change the base?
Conversation
… into YlvasTestingBranch
… into YlvasTestingBranch
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.
Really good job, the site looks great. The only problem seems the responsiveness on the MovieDetails page (on tablet), as you mentioned on in the demo. Your code had a lot of comments, but maybe a little too many comments? It might just be personal preference, but it became a little messy to read at times. Otherwise, everything seems to work well.
/Emma and Nina
useEffect(() => { | ||
fetch(BASE_URL) | ||
.then((data) => data.json()) | ||
.then((configuredData) => setMovieList(configuredData.results)) |
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 mentioned this in the Demo. Maybe because you already used the word 'data' in the .then before, so it just had to be different word here?
<a href="https://github.com/YlvaKarlsson"> | ||
<FontAwesomeIcon icon={faGithub} size="3x" /> | ||
</a> | ||
<a href="https://www.linkedin.com/in/ylvakarlsson87/"> |
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.
Good job with linking to your social media.
// } | ||
|
||
// when replacing the MOVIE_DETAILS in the fetch with the actual link, it finally worked | ||
useEffect(() => { |
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.
No idea why the MOVIE_DETAILS link wouldn't work, but maybe it has to do with how the Id is passed? We didn't try, so not sure.
style={{ backgroundImage: | ||
`url(http://image.tmdb.org/t/p/w1280${details.backdrop_path})` }}> | ||
|
||
<a className="goBackBtn" href="/" onClick={onBackButtonClick}> |
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 button looks good!
element with the .movie-details class is targeted, and not any other descendant elements that may | ||
also have the .movie-details class. */ | ||
|
||
.movie:hover > .movie-details { |
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 did something very similar to this, but we just left out the >, and it works just fine without. So not sure if it's neccesary.
display: none; | ||
} | ||
|
||
.back-to-list: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.
The problem might be, that you've put a hover on something that's been set to "display: none" - you can't hover over something that's not there. Have the text + button in a container instead, and add the hover to the container.
Pair-programming together with Sandra Olsson this week - building a React-app and fetching movie-releases from TMDB:s API.