React Random Quote Machine, seeking feedback

Hello, everyone!

I’d kindly appreciate any kind of feedback on my very first React app:
https://codepen.io/danijels/pen/YzGvjza?editors=0100.
Edit: In Chrome, the tweet button only works if you right-click and select open in a new tab. The app passes all the test, but this kind of bugs me (pun not intended). :thinking:

Thanks in advance for your feedback and happy coding! :grin:

The tweet button is working for me by just left clicking on it.

Now speaking of that tweet button, you’ve coded it as:

<button>
   <a>...</a>
</button>

That’s not valid HTML. Even though you refer to it as a button it is really just a link to twitter, so you should remove the <button> and leave it as just an <a>. Semantics are not based on what something looks like but rather on what its function is.

You’ve also removed the outline property from these “buttons” but haven’t replaced it with anything for their focus states. This is an accessibility issue. Keyboard users should be able to tell where their current keyboard focus is on your page. So you’ll need to add some sort of focus indicator when those buttons are focused by the keyboard.

Your app is fairly responsive to both changes in view port width and text size but I think it can be improved just a little. For example, when I narrow my browser as far as it will go you’ve set the font size for the buttons to 0.5em. Can you read the button text at that size? If so you have really good eye sight :slight_smile: But I can barely make it out and I guarantee you that there are a lot of people who can’t read it at all. There is no need to shrink the text that much for narrow view ports. Personally, I don’t think you need to shrink it at all.

Another example, at a narrow view port you still have a lot of side padding, so the actual quote is only taking up maybe 40% of the page, which means I’m getting 2-3 words per line. You should think about getting rid of most of the side padding and allow the content to have more horizontal room to breath. This will be especially helpful for people who are viewing your page with an increased text size.

A note: the tweet button only works…

Did you try target="_blank".

It looks pretty good. Putting on my nitpicky hat …

let color = colors[randomIndex(colors.length)];

Just say no to global variables. I don’t even think this variable is needed.

For:

        this.setState({
           quote: `"${data.quotes[random].quote}"`,
           author: `- ${data.quotes[random].author}`,
           color: color
        });  

You can just do color instead of color: color - shorthand notation.

With the quote and author, it seems a bit odd to encode the quote marks here - I would rather do those in the JSX. Or at least change it to “quoteString” to represent that it isn’t just the quote. But I think it should just be the quote.

You have duplicated code in componentDidMount and in newQuote. You should just call newQuote from componentDidMount.

      document.body.style.backgroundColor = color;

You shouldn’t be directly manipulating the DOM. You should be using React to adjust style props in the JSX.

Thank you so much for your feedback, especially the parts about styling. It gets a little bit too much for me sometimes, to be perfectly honest.
As for the tweet button - yeah, I need to edit my original post because it works just fine for me too when I try from iOS Safari but not when I left-click in Chrome.

Thank you very much for your reply!
I have tried target="_blank" but it still did not work. I guess it’s not that much of a problem, because apparently it works just fine from some other browsers and an app is not supposed to be used from Codepen, anyways.
Now, about manipulating body style. What should I do there? Would styling the background of the wrapper element be a correct way to do it or should I just use some kind of a head manager?

Now, about manipulating body style. What should I do there?

My instinct would be to do something like:

<div style={ { backgroundColor: color } }>

that surrounds the whole app. You could apply a class to it to stretch it out. I don’t know that something as benign as changing the body color is a problem, but it does make my Spidey-sense tingle.

1 Like

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