salamhis2019 / MI-449-css-spacing-and-layout

0 stars 0 forks source link

Project Feedback #1

Open salamhis2019 opened 2 years ago

salamhis2019 commented 2 years ago

Create a professional-looking personal site or portfolio

@egillespie Can you take a look at this? It's hosted here and meets the following criteria:

egillespie commented 2 years ago

Ooh, nice website and solid use of fetch to show some randomized content. I like it! 🤩

Your use of Flexbox is spot on, and so are the inline-block, padding, and margin on the site. This project covers a lot of ground!

Here are some recommendations for improvement. Can you try these out?

Use display: block on a non-block element

The display: block style on .container is applied to a div element in your HTML. Unfortunately, div elements are already block elements so this style isn't affecting anything.

Can you find (or add) another element to your page that is not a block element, then style it so that it is?

Remove Flexbox style from container

Similarly, since div class="container" is not a Flexbox, the align-items: center style on the .container selector is not doing anything. Can you remove this style?

Show the quote faster, Part 1

Since the Breaking Bad Quote API returns the quotes in a random order every time, you can show the quote on the page faster by getting the first quote from the list instead of using forEach to get every single quote (which ultimately shows the last quote in the list):

quotes.textContent = `"${people[0].quote}"`
author.textContent = `- ${people[0].author}`

My guess is you were practicing using forEach, which is totally understandable, but I did want to point out that this does lead to the quotes taking slightly longer to show. 😉

Show the quote faster, Part 2

Another reason the quotes have a small delay between when the button is pressed and when the quote appears is that the Breaking Bad Quote API is called every time the button is pressed. Downloading information from the Internet takes time, probably more time than the forEach.

If you want to avoid calling the API more than once, I recommend this approach:

  1. Disable the button in HTML by adding the disabled attribute
  2. Call the API as soon as the page loads instead of in a function
  3. In the last .then, do the following:
    1. Assign the quotes to a global variable
    2. Enable the button
    3. Call the function to show a random quote
  4. Change the button function to find a random quote from the global variable

This is a lot of steps, so here's a made-up example of how that would look with some other API:

// Elements on the page
const randomPokemonButton = document.getElementById('random-pokemon')
const pokemonPicture = document.getElementById('pokemon-picture')
const pokemonLabel = document.getElementById('pokemon-label')

// Global list of Pokemon
let allPokemon

// Download the Pokemon list once and save it
fetch('https://api.pokemon.com/all')
  .then(response => response.json())
  .then(results => {
    allPokemon = results
    randomPokemonButton.disabled = false
    showRandomPokemon()
  })

// Show a random Pokemon from the downloaded list
function showRandomPokemon () {
  if (allPokemon) {
    const randomIndex = Math.floor(Math.random() * allPokemon.length)
    const pokemon = allPokemon[randomIndex]
    pokemonPicture.src = pokemon.imgUrl
    pokemonLabel.textContent = pokemon.name
  }
}

randomPokemonButton.addEventListener('click', showRandomPokemon)

See if you can adapt your solution to do this with a random Breaking Bad quote and feel free to reach out if you need pointers!


After you’ve made your changes and pushed them to GitHub and your hosted site, give it a once-over to make sure it looks right, then comment back here and I’ll take another look.

You've got this! 🤠

salamhis2019 commented 2 years ago

Thanks Erik, that's actually something I was really curious about, how I could make the page load faster. Seems like the changes you requested me to make have made a huge improvement on the page loading quicker.

Figured I would play around with an API as I always wanted to learn how to fetch data. Next I want to learn async and await and see the different with that.

egillespie commented 2 years ago

Figured I would play around with an API as I always wanted to learn how to fetch data. Next I want to learn async and await and see the different with that.

I'm a big fan of the async and await syntax! Let me know if you want to try that out here and I'll explain how. 🙂

img is a good choice for styling as a block element and from what I can tell, your JavaScript changes look pretty good. I do have two observations though:

  1. You're no longer using string templates to put " or - around the quote and author
  2. I can't see your changes on your hosted site (this project is hosted on Surge)

Would you mind touching these up and let me know if you want to try out async/await?

Thanks! 🤘🏻

salamhis2019 commented 2 years ago

Went ahead and made those changes, take a look!

egillespie commented 2 years ago

Cool cool cool, I can see the changes working on Surge and they work great. Good job with this project and the additional feedback, I love how it turned out! 🥳 :shipit: