Hi everyone, I`d like some feedback on this app I made. It is a movie app that shows latest movies and gives users to search for any movie they want. You can also maintain your own watchlist by signing in with Google.
I made it using the following technologies-
-React
-Redux
-Firebase(Auth)
-React Spring
-Axios
-Material UI
I have been teaching myself to code for a few months now. I come from a totally unrelated background but would like to make a career switch into the world of Software Development. Would love to hear some tips and advice from you all.
Thanks
This looks really good, and I like that it’s quite minimal in the design. It works really well as a landing page since you know where to look with the eye-catching title and film images.
In terms of the design, I really like it. I think it could be even better if there was more space. Things look squashed together to me.
Another thing that could be improved is the loading. Try to throttle your connection in the network tab of Chrome DevTools, and you’ll notice that the layout can shift as it loads slowly. You already know the dimensions of the images, so maybe you could wrap them in a container to occupy that space before they load.
A few accessibility enhancements for the main page:
When I Tab through the films with my keyboard I do not see any keyboard focus indicator on the links. I think you should add a custom focus indicator.
Likewise, I can barely see the focus indicator on the “Sign In” button. I think you should make it much more visible.
I think the .footer div is just made for a <footer> tag. And the .home div that contains all of the movies would make a great <main> tag.
I don’t think your github link and the “Powered By” link should be <h4>s. These aren’t headings for content. Also, since they come immediately after the last movie <h3> they are technically subheadings for that movie header, so they should be <h2>s instead. But as I said, I don’t think they should be headings at all.
In the movie cards, I would move the <h3> out of the <a>. A screen reader is going to read everything in the link and thus it is going to read both the alt text for the image and the text in the h3, which is needless duplication. Also, I think the <h3> needs to come before the link in the DOM. A lot of people using screen readers will navigate your page by heading and it will be confusing if the link to the movie comes before the heading for the movie. You can still use CSS to visually place the heading below the link.
Furthermore about the movie <h3> headings, I think they need to have the complete title of the movie in the <h3> so that people using screen readers will know what the movie is. For example, a screen reader user is probably not going to know that “Miraculous W” is actually " Miraculous World: Shanghai – The Legend of Ladydragon". You can use CSS to get the ellipses automatically when the title is longer than the space alloted.
I think another option for listing the movies would be to get rid of the <h3> headings and just put the movie cards in a list (semantically I think this makes more sense). And then each item in the list is a link to the movie. The link can include both the image and the name of the movie. To keep screen readers from reading the movie title twice for the link, set the alt text for the image to alt="". Doing this tells the screen reader that the image should be ignored, which is perfectly acceptable in this case since you would also have the complete name of the title in the link as well (using the CSS ellipses trick). If you did it this way then it would not matter whether the image or text came first in the DOM.
For the individual movie page:
The <title> needs to change to reflect the name of the movie being viewed.
The date of the movie should not be an <h4>. It would be an <h2> since it immediately follows an <h1> but I don’t think it should be heading at all.
Again, I don’t think the focus indicator on the “Add to Watchlist” button stands out enough.
For the IMDB and YouTube links, the alt text should not include the word “logo”, it should just be the name of the site it is linking to. Also, use “IMDB” for the alt text so that screen readers will spell out each letter in the acronym.
Thankyou so much for putting in the time to review this.
I noticed the layout shift happening while the images loaded, but didn’t know how to fix it.
Can’t believe the solution to it was as simple as wrapping it in div
Wow! This is the most elaborate feedback I have ever recieved. Cant believe you went the extra mile to point out all of the things that can make this app better.
I have already started working on the updates you suggested. Some of them are so obvious I wonder why I didnt think of them before.
Again, thank you so much for going through it and giving your time to review it.
