Can I get some feedback on my Random Quote Generator project

Code Feedback
1

Here is my Random Quote Generator (https://nedu.github.io/RQG/) and here is the source code (https://github.com/Nedu/RQG). Any feedback is appreciated. @IsaacAbrahamson can you pls help?

2

Sure! I’ll take a look at it tonight if I have time or tomorrow morning.

3

It works fine, and it was interesting the fact that you used fetch

4

Nice work! I’m sorry you hadn’t got any feedback earlier, but I’m glad you saw to mention me! I’ll try to give a quick overview of what I think.

Overall:
Design: 7/10
Code: 9/10

Design:

For design, it is pretty basic. That isn’t necessarily a bad thing, but it doesn’t excite interest. It does the job well though and is functional! A little thing - your buttons at the bottom aren’t centered. Probably the biggest problem is typography - specifically inconsistent font usage. Let’s see how many different fonts you have:

  • logo/title h1
  • the quote itself
  • the author
  • tweet link
  • new quote button

Five different fonts all in one little box. In design, we want to be consistent with our fonts, and we want to use them for a specific purpose. Using a different font for each element is not good practice, and the fonts themselves are varied. The title font is fine, as it is like a logo. Having a script font is fine for the quote as it is depicting a written quote - that is good font usage. The other elements though need to be consistent. You have a serif font for author and tweet, but a san-serif font for new quote. Choose one to use, and stick with it. I would use a san-serif font for those 3 elements.

Here is the difference:

ZHGdbGWESnqAj5N-G134jg.png772Ă—235 2.54 KB

8bRxnK03Sreo56pWABR5pg.png785Ă—247 2.63 KB

You may think I’m being nitpicky, but typography is an often overlooked element for beginners, and this was (and still is) my biggest design problem.

Code:

I was impressed with your GitHub page. I can tell you put care into presenting your work. You showed your mockup (which doing one in the first place is out of the ordinary for most campers here), gave a picture of demo, and provided some information about what you used. Those little things in the README made me want to look at your code! Now to the code itself.

  • Use a linter. There are several spots you have syntax inconsistencies. For example, line 7 you omit a semicolon while on 27 you have two. Using a linter can help you catch mistakes like that.

  • Since you are using HTML5, I would wrap my code in a main element not a div.app.

  • I like the ES6 and formatted code - Nicely done!

  • A little ES6 thing you can add. If you rename

const myHeaders = new Headers({ })

// to

const headers = new Headers({ })

// you can then do this:
fetch(base, { headers })

  • Not sure why you named a URL “base”. Just by looking at it I can’t tell what it is.

  • Regarding your then statements after fetch, did you decide not to use arrow functions? For example, what you have:

fetch(base, {headers: myHeaders})
		.then(function (response) {
			return response.json();
		})
		.then(function (data) {
			displayQuote(data.quote, data.author);
			// displayAuthor(data.author);
		})
		.catch(function () {
			console.log('An error occured');
		});

// can be simplified with ES6 to
fetch(base, { headers })
		.then(response => response.json())
		.then(data => displayQuote(data.quote, data.author))
		.catch(err => console.log(`An error: ${err} occurred`));
  • You have a variable named “a” for your twitter text. This isn’t really helpful at all. Since you are using template strings, you could just do:
tweetQuote.setAttribute('href', `https://twitter.com/intent/tweet?text=${quote}-${author}`);

A final thing you can do for bonus practice. Let’s look at your getQuote() function again (I’ll use my modified version since I have it already here)

function getQuote() {
	fetch(base, { headers })
		.then(response => response.json())
		.then(data => displayQuote(data.quote, data.author))
		.catch(err => console.log(`An error: ${err} occurred`));
}

A slight problem with your code here is that getQuote() doesn’t just get a quote - it also displays it on the screen which is the purpose a different function! This might be considered a naming problem. I mentioned bonus practice, so I’m going to implement this with async functions and use the await keyword. If you haven’t used them yet, it is a good thing you should learn how to do. Here is how I would set it up (with some bonus ES6 destructoring):

async function getQuote() {
	let res = await fetch(base, { headers });
	let data = await res.json()
	return [data.quote, data.author]
}

async function displayQuote() {
	let [quote, author] = await getQuote() 
	const quoteText = document.querySelector('.quote');
	const authorText = document.querySelector('#author');
	const tweetQuote = document.querySelector('.tweet');
	
	quoteText.textContent = quote;
	authorText.textContent = author;
	
	tweetQuote.setAttribute('href', `https://twitter.com/intent/tweet?text=${quote}-${author}`);
}

newQuote.addEventListener('click', displayQuote);
displayQuote();

There are a few things you can research, but the idea is that when you hit the button you want to display a new quote. Your functions should be independent and consistent. getQuote should only get a quote and displayQuote should only display a quote. You want to call displayQuote as it describes what you are doing.

5

Thank you so much for your feedback especially on the design bit because I’m pretty crappy at design. I will take your advice and get to improving my code.

1 Like