CYF-ITP-South Africa | Serna Malala | Module-Data-Groups | Week 3 Carousel Slideshow #286
Conversation
sernamalala
added
📅 Sprint 3
There was a problem hiding this comment.
Code is neat and easy to understand.
-
When "auto forward" and "auto backward" happen concurrently, the slideshow app could behave "weirdly". Can you think of a solution to only allow at most one of them active at any given time?
-
You missed one requirement specified in
readme.md. -
UI/UX: When the user clicks the Forward/Backward button to move from one image to another, because the images have different dimensions, the buttons are displaced accordingly. Can you think of a way to make the buttons stay at the same location?
Sprint-3/slideshow/index.html
Outdated
| <div> | ||
| <button type="button" id="auto-backward-btn">Auto Backwards</button> | ||
| <button type="button" id="backward-btn">Backwards</button> | ||
| <button type="button" id="stop">Stop</button> |
There was a problem hiding this comment.
Why not use a consistent naming convention for all buttons' id value.
There was a problem hiding this comment.
I'll add the word "btn" to stop. I understand what you mean
Sprint-3/slideshow/slideshow.js
Outdated
| index++; | ||
| image.src = imagesArray[index]; | ||
|
|
||
| } | ||
| else { | ||
| index = 0; | ||
| image.src = imagesArray[index]; | ||
|
|
||
| } |
There was a problem hiding this comment.
Can you think of a way (utilising the % operator) to simplify the code in lines 25-32 into 2 lines of code.
There was a problem hiding this comment.
This was a bit tricky to think of, I had to take array lengths and look at how modulo operator works.
index = (index + imagesArray.length + 1) % imagesArray.length;
image.src = imagesArray[index];
cjyuan
added
Reviewed
|
I have made the images be a certain height and width so that the buttons don't move |
|
The only thing that I can see that I missed out in the readme file would be adding the 4th image? I've added a 4th image into my images array. |
|
I already had the clearInterval set up for them to not run at the same time, I've made sure to clear instances and make them null if they should not be running. |
|
You could also consider using only one interval for both auto forward/backward. |
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
I have implemented level 1 and 2 of the slideshow exercise of creating an image carousel
Questions
Ask any questions you have for your reviewer.