ZA | 25-ITP-May | Malusi Skunyana | Sprint 3 | Slideshow #761
Conversation
…n plus stop control
…disabled button states
MalusiS
added
the
Needs Review
aedemarm
added
Review in progress
aedemarm
left a comment
aedemarm
left a comment
There was a problem hiding this comment.
This is a good PR:
- good commit messages
- great separation of concerns (dividing the functionality between functions)
- good job with styling!
Looking at the Acceptance Criteria there is one that is failing:
Given the carousel is auto-advancing
When either the forward or back button is clicked
Then the auto-advance should stop
Please have a go at meeting this requirement.
There is no specific test for this. As an extra challenge you could try adding one or more tests for this functionality!
Sprint-3/slideshow/index.html
Outdated
| <button id="auto-backward">Auto Backwards</button> | ||
| <button id="auto-forward">Auto Forward</button> | ||
| <button id="stop">Stop</button> | ||
| </div> |
There was a problem hiding this comment.
The html updates are logical!
NTH (Nice to Have/ Not required for completion): semantic html
How might you refactor this to use semantic html to improve accessibility and maintainability? (https://www.freecodecamp.org/news/semantic-html5-elements/)
There was a problem hiding this comment.
Thanks for great feedback! I have updated slideshow.js so that clicking the forward or back buttons while auto-advancing now stops the auto-advance as per the acceptance criteria. I also added a test to verify that manual navigation stops auto-advance mode.
There was a problem hiding this comment.
Thanks again for the suggestion. I have now wrapped all slideshow controls (manual and automatic) in a single semantic section with a heading. Manual and auto buttons are visually separated into rows, but screen readers and assistive technologies will now recognize all controls as part of one cohesive group. This improves accessibility, semantic clarity, and maintainability without changing the existing functionality.
|
|
||
| autoForwardBtn.addEventListener("click", () => startAuto(nextImage)); | ||
| autoBackwardBtn.addEventListener("click", () => startAuto(prevImage)); | ||
| stopBtn.addEventListener("click", stopAuto); |
There was a problem hiding this comment.
Good job separating concerns! The individual functions make the code more readable.
There was a problem hiding this comment.
I appreciate all the great feedback you have given.
aedemarm
added
Reviewed
…ntic <section> with heading for accessibility and maintainability
MalusiS
added
Needs Review
aedemarm
left a comment
aedemarm
left a comment
There was a problem hiding this comment.
Well done making the changes: it's working well and your comments about the changes you made to the html make sense.
I have left a few additional comments on this PR to improve it but these are just suggestions and not required.
| }); | ||
| }); | ||
|
|
||
| test("clicking forward or backward while auto-advancing stops auto mode", () => { |
There was a problem hiding this comment.
Good job adding this test!
NTH/Not necessary to fix: separate into two tests
This is testing a couple of pieces of functionality: (1) that when you hit the forward button while auto-advancing, the auto-advancing stops and the carousel moves to the next image and (2) that when you hit the back button while auto-advancing, the auto-advancing stops and the carousel moves to the previous image. In this scenario, it is cleaner to write 2 tests. That way if there are test failures, you can pinpoint more easily exactly why. It is also good practice for maintainability: just like with other code, when a test does too much and becomes too complicated, it becomes difficult to read, understand and maintain for future you and your team.
There was a problem hiding this comment.
This is great feedback! I see how advantageous it would be to write two tests here, instead of having one test that does too much. I really appreciate your valuable and substantive feedback.
| autoBackwardBtn.addEventListener("click", () => startAuto(() => { | ||
| currentIndex = (currentIndex - 1 + images.length) % images.length; | ||
| showImage(currentIndex); | ||
| })); |
There was a problem hiding this comment.
NTH/ Not necessary to do: remove duplication
This code is very similar to the prevImage/nextImage code. In your original PR you reused these methods for the auto functionality, which was good.
Suggestion: You have already seen how you can wrap a function around another to add to it (your startAuto function is a wrapper). Perhaps you could use the same idea for your manual navigation functionality.
aedemarm
added
Complete
3 participants
Learners, PR Template
Self checklist
Changelist
Summary
This PR implements the Image Carousel project with full Level 1 and Level 2 functionality as per the requirements. The carousel now supports both manual and automatic image navigation, with a clean and responsive layout.
Changes Made
HTML (index.html):
JavaScript (slideshow.js):
CSS (style.css):
Testing
Questions
Ask any questions you have for your reviewer.