sjagoori / coination Goto Github PK

Waar worden de cryptocurrencies op geordend? Er staat by market cap in biggest-to-smallest order., maar dat is niet het geval? Staat er cache op? BNB is inmiddels nummer drie met een waarde van $250, terwijl die bij jou nog $59.15 is.

https://github.com/sjagoori/web-app-from-scratch-2021/blob/3dd3060b7a8b6df502937167fe49313ebdf2182e/static/js/modules/rounding.js#L6

Enige plek in heel de code waar je semicolon gebruikt

https://github.com/sjagoori/web-app-from-scratch-2021/blob/3dd3060b7a8b6df502937167fe49313ebdf2182e/static/css/styles.css#L13

Staat er nog een in.

Yo Shabier,

Goeie overview pagina! Je hebt zelfs al detail pagina's, lekker bezig man.
Goed begin van je readme ook.

Je app.js ziet er super clean uit. Modules en al! Lekker functioneel ook.

Je comments met param's zijn ook heel chill!

Je bent al bezig met localStorage ook, heel goed.

Paar dingetjes:

• Er staan nog een paar dingetjes van de fork in je repo. Je zou course, examples en docs weg kunnen gooien, dat maakt het wat cleaner en voorkomt verwarring.

• Vul je readme aan met dingen zoals een screenshot van je overview pagina, een gedeelte over wat voor data je binnen krijgt (Je linkt wel naar docs van de api, maar het kan prettig zijn om een kleine inkijk te geven in wat voor data je gebruikt, hoe het eruit ziet etc.).

• Misschien kun je je functies iets verder opdelen in andere functies. Je functies in render.js zijn super lang.

Verder weinig te melden, het ziet er naar uit dat je het wel snapt ;)
Kijk of je jezelf verder kunt uitdagen als we met routing en templating beginnen de komende weken. Misschien kun je je eigen router of templating engine schrijven. Als je meer uitdaging zoekt en je hebt zelf een idee, stel het gerust voor!

https://github.com/sjagoori/web-app-from-scratch-2021/blob/8dae0430fe37ebe69f4c5523c50464aece6e92cf/spa/index.html#L16

Kan in principe wel gewoon naar <script /> worden aangepast.

https://github.com/sjagoori/web-app-from-scratch-2021/blob/8dae0430fe37ebe69f4c5523c50464aece6e92cf/spa/static/js/modules/api.js#L5

Dit valt wel in de categorie mierenneuken, maar volgens mij hoort er (officieel) een enter tussen params en de rest van de comment te zitten. Correct me if I'm wrong, haha. Heeft weer met readability te maken.

Repository

• Scan your buddy's repository
• Is there a repo description at the top?
• Is the repo open to issues?
• Does it include a gitignore
• Is the concept described well in the readme?
• Is the API described in the readme?

Nice and clear work men!

Code

• Read the code

• Try to understand the code flow

• Check the code against our course's best practices

• Check the code for consistency, is code being written the same way in different places? --> you are using params, nice! Try to use params consistent at each function ;). You are missing it at app.js

• Does the code follow naming conventions? Do variable and function names make sense to you? --> Try to describe your function name more descriptive. It's okay now, but it can be more descriptive, I think :).

Top:

• You work with ES6 modules, nice!

Tip:

• This can be written in more lines in stead of one line. It's hard to understand and read.

  let infoList = await getData('https://shacors.herokuapp.com/https://pro-api.coinmarketcap.com/v1/cryptocurrency/info?id=' + topList.data.data.map(item => item.id).toString() + '&CMC_PRO_API_KEY=a50e1e84-9b13-4d34-86e5-8b386c49c31c')

• You are writing code comments in params for functions, cool! But write it also for the variables, that makes it more clear!

Good job buddy! 🚀 🚀 🚀

Niet echt een issue, maar is het niet beter om deze in je .gitignore te zetten? Lijkt mij dat het jouw voorkeur is, toch?

Wellicht kan test.js nog weg. Doet naast een console.log niet zo veel.

Als het wel wat doet, dan kun je misschien de naam aan passen. Is nu een beetje raar.

Dat was het laatste. Voor de rest kan ik niet zo veel vinden 😄

https://github.com/sjagoori/web-app-from-scratch-2021/blob/8dae0430fe37ebe69f4c5523c50464aece6e92cf/spa/static/js/modules/render.js#L10

Je moet zelf maar even kijken of je het met mij eens bent, maar ik vind de functie nogal lang. Zou je deze niet kunnen opsplitsen.

Je zou ook kunnen kijken naar het schrijven van HTML in JS. Maakt het volgens mij ook een stuk korter.

Moet je maar even Googelen naar Write JS in HTML JSX oid als je niet weet hoe of wat. Kom je wel uit.

Ik heb het zelf zo gedaan

https://github.com/sjagoori/web-app-from-scratch-2021/blob/3dd3060b7a8b6df502937167fe49313ebdf2182e/static/js/modules/colors.js#L2

Wellicht een idee om in een comment erboven even alle kleuren toe te lichten. Geen idee in hoeverre het relevant is om te weten, maar ik weet nu niet om welke kleuren dit gaat.

Ontzettend goed bezig! Tof hoever je al bent en hoe netjes de code is. Ik zie hier iemand die consistent werkt, nice :)

Een paar tips:

• Kun je jezelf nog wat meer uitdagen in het concept? Hoe kan je deze app complexer maken? Er is een probleem rondom crypto currency wat je zou kunnen oplossen met jouw app? Bijvoorbeeld... zouden we kunnen voorspellen wat over 10 jaar de top crypto's zijn, of kun je misschien een vergelijking maken tussen crypto en aandelen?
• Voeg hier en daar code comments toe, wat doet welke functie en waarom. Bijvoorbeeld: ({ ...infoList.data.data, merged }, delete infoList.data.data.merged)
• Nice grid in CSS!
• Complimenten voor de modules en hoe clean dit is opgezet!
• Is het bouwen van je eigen template engine een idee?

Hele interessante API Shabier! Zoals ik al verwacht had is je code heel strak en heb je overal comments bijgezet om het voor iedereen leesbaar te maken, super chill!