Code review #1

xtopolis · 2022-06-23T03:15:04Z

CSS

isn't auto the default value? => quote-generator/css/style.css Line 83 in a449eaa width: auto; #2
dead code, do not eat =>

quote-generator/css/reset.css

Line 38 in a449eaa

/* line-height: 1.5; */
--space is not defined? =>

quote-generator/css/style.css

Line 43 in a449eaa

margin-block-start: var(--space, var(--15px));
unused rule? =>

quote-generator/css/style.css

Line 67 in a449eaa

.fs-64px {
While this is not wrong, I'd argue a class is more appropriate than using data-type attribute and targeting that just for styling, I'd say the data-long is OK, but data-type isn't... also see below, maybe using custom data attribute isn't needed =>

quote-generator/css/style.css

Line 130 in a449eaa

button[data-type="twitter"]:hover {
1024px is a more common breakpoint for tablet~ but this is up2u =>

quote-generator/css/style.css

Line 143 in a449eaa

@media screen and (max-width: 1000px) {
Overall: vars with names like --10px are descriptive, but maybe not canonical~ usually you'd make them prescriptive names: Heading, Subhead, Body, ... There's nothing to change, but to keep in mind for future reference

HTML

Missing meta description tag =>

quote-generator/index.html

Line 3 in a449eaa

<head>

Javascript

I'd argue show/removeLoadingSpinner can be refactored into one function passing a Boolean as an argument =>

quote-generator/script.js

Line 15 in a449eaa

const removeLoadingSpinner = () => {
It's often bad practice to show users the developer facing errors (${error}), instead omit that from the template literal and log it instead =>

quote-generator/script.js

Line 31 in a449eaa

alert(`Whoops, something went wrong. ${error}`);
Array access is constant/instant (in memory), loading spinner methods shouldn't be called inside newQuote IMO. You could make a transition method with a timer, but as it is now, it "doesn't do anything" because the next quote is instant =>

quote-generator/script.js

Line 37 in a449eaa

showLoadingSpinner();
This block is perfectly correct, but we often use nullish coalescing ??, if you're into that =>

quote-generator/script.js

Line 44 in a449eaa

if (!quote.author) {
This is fine, but this could also be a class toggle maybe instead , 50/50 =>

quote-generator/script.js

Line 51 in a449eaa

if (quote.text.length > 120) {

Overall

🎉 Good use of .textContent to avoid xss =>

quote-generator/script.js

Line 58 in a449eaa

quoteText.textContent = quote.text;
Minor: In larger projects you'd want to extract API urls or "magic numbers" into variables (ex: magic number = 120 = LARGE_QUOTE_LENGTH), but ok for this small project
I'd still advise wrapping your code in an IIFE, even though it technically wouldn't change anything => https://developer.mozilla.org/en-US/docs/Glossary/IIFE

The text was updated successfully, but these errors were encountered:

vivian-mca · 2022-06-24T02:21:11Z

@xtopolis

CSS

Line 83: Deleted width: auto
Line 38: Deleted dead code (line-height: 1.5)
I left --space there in case I wanted to make flow exceptions, but since I'm not using it, I deleted it.

quote-generator/css/style.css

Line 43 in a449eaa

margin-block-start: var(--space, var(--15px));
Deleted --fs-64px

While this is not wrong, I'd argue a class is more appropriate than using data-type attribute and targeting that just for styling, I'd say the data-long is OK, but data-type isn't... also see below, maybe using custom data attribute isn't needed =>

quote-generator/css/style.css

Line 130 in a449eaa

button[data-type="twitter"]:hover {

For that one ↑, I learned using data-state attribute due to this documentation. I do agree that data-type doesn't seem to fit, so I change it to a class="button-twitter instead.

Changed breakpoint to 1024px

Overall: vars with names like --10px are descriptive, but maybe not canonical~ usually you'd make them prescriptive names: Heading, Subhead, Body, ... There's nothing to change, but to keep in mind for future reference

I used to implement the 62.5% trick so each rem = 10px. However, someone pointed out to me that it causes compatibility issues. So I used this method instead: https://www.joshwcomeau.com/css/surprising-truth-about-pixels-and-accessibility/#leveraging-css-variables. Regarding giving them prescriptive names, how should I name them if I reuse them for other blocks unrelated to their names?

xtopolis · 2022-06-24T03:32:57Z

I used to implement the 62.5% trick so each rem = 10px. However, someone pointed out to me that it causes compatibility issues. So I used this method instead: https://www.joshwcomeau.com/css/surprising-truth-about-pixels-and-accessibility/#leveraging-css-variables. Regarding giving them prescriptive names, how should I name them if I reuse them for other blocks unrelated to their names?

This is not my area of expertise. My original comment was more in line with using them differently (which would make more sense in a larger project)

html {
  --14px: 0.875rem;
  --15px: 0.9375rem;
  --16px: 1rem;
  --17px: 1.0625rem;
  --18px: 1.125rem;
  --19px: 1.1875rem;
  --20px: 1.25rem;
  --21px: 1.3125rem;
}
h1 {
    font-size: var(--21px);
}

// use the vars in classes or existing elements with more semantic names
h2 {
    font-size: var(--20px);
}
h3 {
    font-size: var(--19px);
}

p {
    font-size: var(--14px);
}

.authorName {
    font-size: var(--14px);
}
.quoteText {
    font-size: var(--62px);
}

vivian-mca · 2022-06-25T04:01:29Z

@xtopolis

HTML

✅ Added meta description

JS

✅ Refactored showLoadingSpinner function
https://github.com/vivian-mca/quote-generator/blob/gh-pages/script.js#L10
✅ Removed error from showing in alert(), logged it instead
https://github.com/vivian-mca/quote-generator/blob/gh-pages/script.js#L31
✅ Refactored using ??
https://github.com/vivian-mca/quote-generator/blob/gh-pages/script.js#L44

xtopolis · 2022-06-25T21:40:01Z

JS

✅ Refactored showLoadingSpinner function

We can go deeper

const showLoadingSpinner = (isHidden) => {
    loader.hidden = !isHidden;
    quoteContainer.hidden = isHidden;   
};

vivian-mca · 2022-06-25T23:45:12Z

@xtopolis

✅ I refactored showLoadingSpinner function with your code.
✅ I wrapped async function in IIFE.

Question on IIFE:

Is it common/standard practice to wrap async functions in IIFE?
What are commonly wrapped in IIFE?

xtopolis · 2022-06-26T17:29:06Z

Question on IIFE:

Is it common/standard practice to wrap async functions in IIFE?

What are commonly wrapped in IIFE?

You wrap your entire script file in the IIFE, not just this function.

The use of IIFE doesn't have an impact on your project, it's just a teaching comment

If you were to share this script with someone as is, you'd be leaking/potentially-conflicting these variables into the global scope:

const quoteContainer = document.getElementById("quote-container");
const quoteText = document.getElementById("quote");
const authorText = document.getElementById("author");
const twitterBtn = document.getElementById("twitter");
const newQuoteBtn = document.getElementById("new-quote");
const loader = document.getElementById("loader");
//-------
let apiQuotes = [];
const showLoadingSpinner()...
const getQuote() ...
function newQuote() ....
const tweetQuote() ...

If we ignore the DOM selectors at the top for now (we'd refactor this/etc before sharing), you'd still have 5 javascript variable/function definitions with not-very-unique names that have a high chance of conflicting with other scripts on a page.

By wrapping it in an IIFE, the code still gets executed but in the function scope such that the variables aren't visible outside of that wrapped code (() => wrapped code)().

If you read the section on the module pattern, it might make more sense about how the variables are accessible within that function, but not to outsiders who try to access the "private variables" like .balance. It also shows that your entire script could be anonymous, or reveal only a top-level Object with various properties to call functions you expose, reducing your footprint from N variables to 1 variable in the global scope, and that the consumer could possibly rename if they so desired (they can assign the return value of the IIFE to whatever variable name they want)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Code review #1

Code review #1

xtopolis commented Jun 23, 2022 •

edited by vivian-mca

Loading

vivian-mca commented Jun 24, 2022 •

edited

Loading

xtopolis commented Jun 24, 2022

vivian-mca commented Jun 25, 2022

xtopolis commented Jun 25, 2022

JS

vivian-mca commented Jun 25, 2022

xtopolis commented Jun 26, 2022 •

edited

Loading

Code review #1

Code review #1

Comments

xtopolis commented Jun 23, 2022 • edited by vivian-mca Loading

CSS

HTML

Javascript

Overall

vivian-mca commented Jun 24, 2022 • edited Loading

CSS

xtopolis commented Jun 24, 2022

vivian-mca commented Jun 25, 2022

HTML

JS

xtopolis commented Jun 25, 2022

JS

vivian-mca commented Jun 25, 2022

xtopolis commented Jun 26, 2022 • edited Loading

xtopolis commented Jun 23, 2022 •

edited by vivian-mca

Loading

vivian-mca commented Jun 24, 2022 •

edited

Loading

xtopolis commented Jun 26, 2022 •

edited

Loading