ITP London_Jan 2025| Samunta Sunuwar | Module Data Groups | Sprint 3| Slideshow #515
Conversation
Samunta
commented
Apr 22, 2025
Samunta
commented
- I have committed my files one by one, on purpose, and for a reason
- I have titled my PR with COHORT_NAME | FIRST_NAME LAST_NAME | REPO_NAME | WEEK
- I have tested my changes
- My changes follow the style guide
- My changes meet the requirements of this task
illicitonion
added
Review in progress
| <button type="button" id="backward-btn">Backwards</button> | ||
| <button id="stop">Stop</button> | ||
| <button type="button" id="forward-btn">Forward</button> | ||
| <button type="button" id="auto-backward">Auto Backward</button> |
There was a problem hiding this comment.
I found it a little surprising that "Auto Backwards" is in the "Forwards" direction end of the buttons, and "Auto Forwards" was in the "Backwards" direction end of the buttons. Is there a reason you ordered them like this? Would it perhaps be more clear if you swapped them?
| function startAutoForward() { | ||
| stopSlideshow(); | ||
| slideshowInterval = setInterval(goForward, 2000); | ||
| autoForwardBtn.disabled = true; | ||
| autoBackwardBtn.disabled = true; | ||
| } | ||
|
|
||
| // Auto Backward logic | ||
| function startAutoBackward() { | ||
| stopSlideshow(); | ||
| slideshowInterval = setInterval(goBackward, 2000); | ||
| autoForwardBtn.disabled = true; | ||
| autoBackwardBtn.disabled = true; | ||
| } |
There was a problem hiding this comment.
These two functions seem to mostly do the same thing.
Imagine if we decided we also wanted to disable the "Forward" and "Backward" buttons when in auto mode, and only allow the "Stop" button to be pressed. We would need to make the same change to both of these functions. Maybe we wouldn't remember this, and would only change one, and have a bug.
Can you think of a way to avoid this, so that we re-use this code across both functions, rather than having two copies of it?
| let currentIndex = 0; | ||
| const carouselImg = document.getElementById("carousel-img"); |
There was a problem hiding this comment.
In general your naming in this PR is really good. The variables are really clear what they are and what they're for, the functions have really clear names - I can tell what they do from just their names, etc. Good job!
illicitonion
added
Reviewed
