NW6 | Fathi Kahin | JS2 | slideshow | Week 3 #195
Conversation
✅ Deploy Preview for cute-gaufre-e4b4e5 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Good work FathI! INteresting structure as well. For the future, try to name elements something that is easy to understand rather than something generic like num as this doesnt tell the reviewer much intformation about what it is exatly>
Another thing, in the final demo they showed, they have a counter of which image you are on. I didnt manage to find that feature in the instructions and your code doesnt hav that feature implemented. Feel free to implement it and we can look over again>
Well done!
| <meta charset="UTF-8" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
| <link rel="stylesheet" href="/week-3/slideshow/style.css"> | ||
| <title>Slideshow</title> |
There was a problem hiding this comment.
Be very careful when readinginstructions! The title should have been image carousel!
| let num = 0; | ||
| imageSelector.style.width = "600px"; |
There was a problem hiding this comment.
i would definitely do this in css! It works here, you might just want to keep JS seperate for functionality
| let num = 0; |
There was a problem hiding this comment.
I am nitpicking here, but in a perfect world, we dont want to have global variables. ALl the variables you want to use should be defined in a function. Agsin this works fine, just a note for later!
| num++; | ||
| if (num === images.length) { | ||
| num = 0; |
There was a problem hiding this comment.
Im not quite sure what num is supposed to be. For the future, tru to name variables something people can understand just by looking at your code!
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.