Yolandaokyere / ArtLibrary

ArtLibrary is a single page web app. The user can enjoy National art and info of the Rijksmuseum collection.
https://yolandaokyere.github.io/ArtLibrary/Rijksmuseum
MIT License
0 stars 0 forks source link

Peer Review - Julian de Cloe #1

Open juliandecloe opened 2 years ago

juliandecloe commented 2 years ago

Er onstaat in jouw javascript code een structuur waarbij je van links naar rechts werkt. Er ontstaat steeds meer witruimte links van de regels. Probeer een beetje op inspringen te letten. Dat houdt het overzichtelijker voor jezelf.

Daarnaast kun je beter de fetch in een function zetten en dan die function aanroepen. Dan kun je namelijk makkelijk event handlers eraan toevoegen. Een voorbeeld zou zijn dat de fetch pas gebeurd als er een knop wordt ingedrukt.

Het zou er dan zo uit kunnen zien:

const endpoint = 'https://www.rijksmuseum.nl/api/nl/collection/?key=GnjcnmeH'; 

myFunction()

function myFunction() {
     fetch (endpoint)
     .then(function(response){
         return response.json()
     })
     .then(function(rijksmuseumData) {
            console.log(rijksmuseumData);
            console.log(rijksmuseumData.artObjects[0].webImage.url)

            const artObjects = rijksmuseumData.artObjects

            for (let i = 0; i <745519; i++) {
                    const  kunstimg = rijksmuseumData.artObjects[i].webImage.url
                    document.querySelector('ul').insertAdjacentHTML(`beforeend` , `<li><img src="${kunstimg}"></li>` )   
                    document.body.style.backgroundColor = 'red';  
             }
       })
}

Je maakt een const aan van artObjects, maar die gebruik je vervolgens niet. Je gebruikt die rijksmuseumData.artObjects i.p.v alleen de artObjects. Nu heb je een const gemaakt, maar gebruik je hem niet.

Je 'for loop' gaat bijna goed. Een loop heeft altijd deze structuur:

for (let i = 0; i < element.length ; i++) {
    content  
}

die element.length bepaalt de lengte van de array (element is in jouw geval artObjects). Dus dan wordt het: 'artObjects.length'. Dan kun je vervolgens in de content artObjects[i] doen (dit deed je wel goed) om de hele array aan te roepen.

Manier om images kleiner uit de API te halen: artObjects.webImage.url.slice(0, -3)+"=s1000"

Yolandaokyere commented 2 years ago

Hey bedankt voor je feedback! Had niet door dat ik steeds meer ruimte maakte heb dit nu gefixt, tis nu idd meer overzichtelijk nu.

Daarnaast kun je beter de fetch in een function zetten en dan die function aanroepen. Dan kun je namelijk makkelijk event handlers eraan toevoegen. zou je mij dit morgen nog even kunnen laten zien hoe je dat doet!

Yeah bedankt voor je geweldige tip heb nu mijn images kunnen optimaliserenšŸ„°