xtopolis commented 2 years ago

CSS

[x] #2
[x] dead code, do not eat => https://github.com/vivian-mca/quote-generator/blob/a449eaa0a7f0cc33cd60a7240f5dce12b807e526/css/reset.css#L38
[x] --space is not defined? => https://github.com/vivian-mca/quote-generator/blob/a449eaa0a7f0cc33cd60a7240f5dce12b807e526/css/style.css#L43
[x] unused rule? => https://github.com/vivian-mca/quote-generator/blob/a449eaa0a7f0cc33cd60a7240f5dce12b807e526/css/style.css#L67
[x] 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 => https://github.com/vivian-mca/quote-generator/blob/a449eaa0a7f0cc33cd60a7240f5dce12b807e526/css/style.css#L130
[x] 1024px is a more common breakpoint for tablet~ but this is up2u => https://github.com/vivian-mca/quote-generator/blob/a449eaa0a7f0cc33cd60a7240f5dce12b807e526/css/style.css#L143
[x] 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

[x] Missing meta description tag => https://github.com/vivian-mca/quote-generator/blob/a449eaa0a7f0cc33cd60a7240f5dce12b807e526/index.html#L3

Javascript

[x] I'd argue show/removeLoadingSpinner can be refactored into one function passing a Boolean as an argument => https://github.com/vivian-mca/quote-generator/blob/a449eaa0a7f0cc33cd60a7240f5dce12b807e526/script.js#L15
[x] It's often bad practice to show users the developer facing errors (${error}), instead omit that from the template literal and log it instead => https://github.com/vivian-mca/quote-generator/blob/a449eaa0a7f0cc33cd60a7240f5dce12b807e526/script.js#L31
[x] 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 => https://github.com/vivian-mca/quote-generator/blob/a449eaa0a7f0cc33cd60a7240f5dce12b807e526/script.js#L37
[x] This block is perfectly correct, but we often use nullish coalescing ??, if you're into that => https://github.com/vivian-mca/quote-generator/blob/a449eaa0a7f0cc33cd60a7240f5dce12b807e526/script.js#L44
[x] This is fine, but this could also be a class toggle maybe instead , 50/50 => https://github.com/vivian-mca/quote-generator/blob/a449eaa0a7f0cc33cd60a7240f5dce12b807e526/script.js#L51

Overall

🎉 Good use of .textContent to avoid xss => https://github.com/vivian-mca/quote-generator/blob/a449eaa0a7f0cc33cd60a7240f5dce12b807e526/script.js#L58
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

vivian-mca commented 2 years ago

@xtopolis

CSS

[x] Line 83: Deleted width: auto
[x] Line 38: Deleted dead code (line-height: 1.5)
[x] I left --space there in case I wanted to make flow exceptions, but since I'm not using it, I deleted it. https://github.com/vivian-mca/quote-generator/blob/a449eaa0a7f0cc33cd60a7240f5dce12b807e526/css/style.css#L43
[x] 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 => https://github.com/vivian-mca/quote-generator/blob/a449eaa0a7f0cc33cd60a7240f5dce12b807e526/css/style.css#L130
[x] 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.
[x] 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
[x] 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 commented 2 years ago

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 commented 2 years ago

@xtopolis

HTML

:white_check_mark: Added meta description

JS

:white_check_mark: Refactored showLoadingSpinner function https://github.com/vivian-mca/quote-generator/blob/gh-pages/script.js#L10
:white_check_mark: 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 commented 2 years ago

JS

✅ Refactored showLoadingSpinner function

We can go deeper

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

vivian-mca commented 2 years ago

@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 commented 2 years ago

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)

vivian-mca / quote-generator

Code review #1

CSS

HTML

Javascript

Overall

CSS

[x] Deleted `--fs-64px`

[x] 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.

[x] Changed breakpoint to `1024px`

HTML

JS

JS

vivian-mca / quote-generator

Code review #1

CSS

HTML

Javascript

Overall

CSS

[x] Deleted --fs-64px

[x] 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.

[x] Changed breakpoint to 1024px

HTML

JS

JS

[x] Deleted `--fs-64px`

[x] 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.

[x] Changed breakpoint to `1024px`