Can I get some feedback on my Random Quote Generator project

Can I get some feedback on my Random Quote Generator project
0.0 0

#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:

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.