Random Quote Machine React project feedback

Project Feedback
#1

I finished the Random Quote Machine project and I am looking to get some feedback on it. Links:

You can notice I used Context API for my project which was probably unnecessary, but I just wanted to try it out. I also added some additional functionality which wasn’t part of the challenge, I enabled the user to check the previous quote as well.

I have an additional questions regarding deploying the project to GitHub pages. Can I upload the project to GitHub pages, but have the normal code in my repo not the one created with the build? Because right now with the build code you can’t really look at the code of the project in my repo and understand what is going on. I couldn’t find on my own how to do this (If it is possible).

Thank you for your answers in advance.

Edit:
It seems like I have figured out how to deploy the code to my GitHub as well (I just had to create a new branch and push it there), so now you can see the code on my GitHub or at the CodeSandbox.

#2

100%. If this were a production app, then yeah, that would be overkill, but for learning? Definitely.

Looking that the UI, looks good to me.

Looking at the code…

It looks pretty good.

I see things like:

function App() {
  return (
    <div id="wrapper">
      <div id="quote-box">
        <Quote />
        <Buttons />
      </div>
    </div>
  );
}

Why is a div nested in a div? Couldn’t those just be one div?

Something like this:

<button id="prev-quote" onClick={() => previousQuote()}>

Can usually be shortened to:

<button id="prev-quote" onClick={ previousQuote }>

If you’re not passing explicit parameters (or are using the default that are being passed), you can just pass the function by reference, not needing to wrap it in another function.

This:

  const { previousQuote } = useContext(Context);
  const { setClick } = useContext(Context);
  const { quote } = useContext(Context);
  const { author } = useContext(Context);

Couldn’t that just be:

  const { previousQuote, setClick, quote, author } = useContext(Context);

  const prevQuoteRef = useRef();
  const prevAuthorRef = useRef();

Using a ref seems like overkill to me here. I may be wrong, but it seems like you could find a simpler way to store those values in the module.

But all in all, it looks pretty good. I had to get really nitpicky to find anything to kvetch about.

Have fun with the markdown previewer.

1 Like
#3

Overall this is nicely done but there are a few things to point out:

  • I highly recommend you customize the keyboard focus outline. You are relying on the browser default and I cannot see the outline at all with my browser.
  • The twitter link needs to have actual text associated with it or screen reader users will not know what the link is for. Add “Twitter” to the <a> as you normally would but visually hide the text so it doesn’t display on the page.
  • Consider using a <blockquote> for the quote. Check out MDN’s blockquote example which wraps it in a <figure>.
1 Like
#4

Thank you for your thorough reply.

Regarding the div nesting there is really no reason for that as you have mentioned. Initially I created the entire app in just the App component and only later on created two additional components for practice, but I overlooked that I was nesting a div for no good reason.

I have a habit that whenever I create an onClick event I pass an anonymous function to it, which console.logs somethings, just to check that it works ok, so I end up with something like:

<button id="prev-quote" onClick={() => console.log("OK")}>

Naturally when I create the actual function I want to call, I completely forget I don’t need that anonymous function anymore. :slight_smile:

And finally I most definitely can shorten the code that I have written for the useContext hook the way that you recommended.

Initially I wasn’t thinking about using a ref, but I figured since I never used it before, might as well try it out.

Thank you again for your detailed review, I will implement your recommendations to my code.

#5

Yeah, that happens sometimes. I have a habit of when the code is “done”, I like to go back and look for things I can do to simplify.

I have a habit that whenever I create an onClick event I pass an anonymous function to it, which console.logs somethings, just to check that it works ok, …

Yup, I do that too. But I also try to simplify things. Number one, it makes for cleaner code. And secondly, creating anonymous functions directly in your JSX can cause unnecessary rerenders in some cases - I try to avoid them, at least in the final code.

Initially I wasn’t thinking about using a ref, but I figured since I never used it before, might as well try it out.

I get that, but I think that refs in general are a very heavy handed solution. I think the React docs actively discourage it unless absolutely necessary. Just beware.

1 Like
#6

Thank you for your answer, I forgot about making sure my app is properly accessible for all users. I checked out the outline myself and I could barely see it.

Regarding the blockquote is the main benefit of using it, because it makes your HTML more semantic or are there also some other benefits that I am missing?

Thank you for your tips, I will do the changes so my app will be more accessible.

#7

I did not know that, thanks for the warning.

#8

This topic was automatically closed 182 days after the last reply. New replies are no longer allowed.