Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SOUTH AFRICA CPT-ITP | LUKE MANYAMAZI | MODULE DATA GROUPS | SLIDE SHOW | WEEK 3 #278

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Luke-Manyamazi
Copy link

@Luke-Manyamazi Luke-Manyamazi commented Jan 7, 2025

Learners, PR Template

Self checklist

  • 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

Changelist

Briefly explain your PR.

Questions

Ask any questions you have for your reviewer.

@Luke-Manyamazi Luke-Manyamazi added the Needs Review Participant to add when requesting review label Jan 7, 2025
@Luke-Manyamazi Luke-Manyamazi linked an issue Jan 7, 2025 that may be closed by this pull request
5 tasks
@halilibrahimcelik halilibrahimcelik self-requested a review January 8, 2025 21:08
@halilibrahimcelik halilibrahimcelik added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Participant to add when requesting review labels Jan 8, 2025
Copy link

@halilibrahimcelik halilibrahimcelik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work! You've successfully implemented all the requirements outlined in the README file. While styling wasn't a primary focus, I must commend your clean and thoughtful approach. The carousel, complete with hover effects, is well-executed and visually appealing.

Here's a challenge for you: consider adding a feature that pauses the sliding animation when the user hovers over the slides. This would enhance user interaction and make your implementation even more dynamic.

dots[slideIndex].className += " active";
}

document.querySelector('.prev').addEventListener('click', () => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a common practice that while selecting your html elements, it is better to organize them at the beginning of your script. You defined your img element at the beginning, also define other elements at the same place.

@Luke-Manyamazi
Copy link
Author

Luke-Manyamazi commented Jan 9, 2025 via email

@halilibrahimcelik halilibrahimcelik added Reviewed Volunteer to add when completing a review and removed Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. labels Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Reviewed Volunteer to add when completing a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TECH ED] Build slideshow app
2 participants