Random Quote Machine [FCC]

Code Feedback
1

Hey Guys, I just finished my Random Quote Machine Project using React. I would like you all to see it and let me know how I can improve it. Let me know my mistakes.
Link: https://7z3f3.csb.app/
Thank you!..

2

I wasn’t able to get it to work either.

I found someone that was solving by adding this to the bottom of the src/index.js file:

// Append FreeCodeCamp's Test Runner
const scriptDiv = document.createElement('script')
scriptDiv.src =
  'https://cdn.freecodecamp.org/testable-projects-fcc/v1/bundle.js'

document.body.appendChild(scriptDiv)
// End of Append

That worked for me. It’s a little hinky to do that in a React app (in general, you shouldn’t be manipulating the DOM directly), but if that’s what works for this isolated case…

1 Like
3

Looking at your code…

It looks pretty good. I have a few thoughts.

App.js:

const [isLoaded, setIsLoaded] = useState(null);

To me, the name “isLoaded” is a boolean. I would default it to false, not null.

Why have a default quote? Why not just load the quotes and select?

Is quoteArray an array? It looks like an object to me. But to me, I would expect it to just be an index, like quoteIndex.

Do you ever clear your error? What happens if you get an error and refetch? I would clear the error as the last step in the final then of the fetch.

  if (error) {
    return <div> Error: {error.message}</div>;
  } else if (!isLoaded) {
    return <div> Loading...</div>;
  } else {
    //Method to change quote, author and certain UI components colors.
    const randomIndex = () => {

There is no need for elses here. Since the first condition is returning, it will never get to the second if if the first one was true. A cleaner way to write this is:

  if (error) {
    return <div> Error: {error.message}</div>;
  }

  if (!isLoaded) {
    return <div> Loading...</div>;
  }

  //Method to change quote, author and certain UI components colors.
  const randomIndex = () => {

That is a pretty common pattern. Anytime I can get rid of some unnecessary nesting is a good thing.

const randomIndex = () => {

I think this is a bad name for a function. When I read that name, I think it is a value, not a function. For functions, I want the name to tell me what it does. I would want something like “getNewIndex” or something like that.

I don’t like that your random index could have the same number twice in a row (or 3 or more). It is possible to right a function to return a non-repeating number, but that may be more complicated than you want to do. I mean, this is fine for this, but these are the kind of things that developers should think about when they are programming, “What could go wrong?”

        <QuoteText
          quote={quoteArray.quote}
          author={quoteArray.author}
          handler={randomIndex}
        />

A few things here. First, why not just pass in the quoteArray? (But again, not an array). I would just pass in the whole object. Anytime I can easily move complexity downstream is a good thing. Parent components sometimes get cluttered. Let the child figure out what it needs.

Also, I think “handler” is an unnecessarily vague name in this case. What about “selectNewQuote” or “getNewQuote”? It may sound trivial, but when you’re working on a 100k line app, little things like this will drive you crazy.

This:

isHydrating={true}

For props that are always true, you can use:

isHydrating

React interprets that as true.

Link.js

Does that div do anything?

QuoteText.js

I don’t like the name of this. It is the text and the reload button. It’s a nitpicky thing, but I think a name should tell you exactly what it is. I would want to call this something like QuoteBox or something more generic like that, if the button is going to be in there.

But still, it looks good. It’s cleaner than I remember my React quote app being.

Keep up the good work. Have fun with the Markdown Previewer.

1 Like
4

@kevinSmith did a great job of feedback on your code. I’ll try to do the same for the UI.

  • Both the button and tweet link need to have actual text associated with them so that people who are listening to your page know what they do. For the button it would be something like “New Quote” and for the link it would be something like “Tweet Quote”. I would recommend you add text to them but visually hide it so that it doesn’t show up on the page (but screen readers will still hear it).

  • The tweet link doesn’t quite do what it is supposed to do. When you click on that link it should not only take you to twitter but also fill in the quote and author for you. Take a look at the FCC example for how to do this.

  • Your design isn’t responsive to narrow view port widths. Narrow your browser in as far as it will go and you’ll see a horizontal scroll bar. You will definitely want to adjust your CSS so that this doesn’t happen. In the future, I would suggest you use a narrow-first approach to styling. Narrow your browser as far as it will go and style the page to look good at this narrow width. This will be your default/base CSS. Then you can widen and make adjustments for wider view ports as needed using media query break points if needed.

  • I think that a <blockquote> would be appropriate for this since it is a project that merely displays quotes. Check out how MDN marks up their quotes.

  • A few of the colors you use make it really hard to see the icon on the button (for example the dark blue). I would either pick a lighter shade or make the icon white for these colors.

  • There is a problem with the focus indicator on the tweet link. If you are using the keyboard and Tab to the new quote button you’ll notice an outline ring appears around the button, which indicates that the keyboard focus is currently on the button. But if you Tab one more time to the tweet link you will not see the focus indicator. Also, I would customize the focus indicator using the CSS outline property on the :focus pseudo-class so that it stands out a little more since some browser defaults aren’t very good.

1 Like
6

I have made the changes.
Thank you @kevinSmith and @bbsmooth for your advice. It was really helpful!.:innocent:

1 Like
7

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