Skip to content

London | 25-ITP-May | Houssam Lahlah | Sprint 3 | Slide show #751

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

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

Conversation

HoussamLh
Copy link

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with Region | Cohort | FirstName LastName | Sprint | Assignment Title
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

## Changelist

What I did:

  • Built a JavaScript-powered image slideshow.

  • Implemented manual navigation using Next and Previous buttons.

  • Added automatic slideshow functionality that cycles through images at a set interval.

  • Ensured the slideshow loops correctly when reaching the start or end.

  • Used setInterval and clearInterval to manage timing and prevent conflicts.

## Questions

  • Do you think the code handles both manual and automatic slideshow smoothly?

  • Is the interval management clear and easy to maintain?

  • Any improvements for better accessibility or responsiveness?

HoussamLh added 11 commits July 25, 2025 20:42
- Changed console.log to use address.houseNumber instead of address[0]
- Ensures correct display of house number in output
- Replaced invalid for...of loop on object with Object.values()
- Ensures all property values of author are logged without errors
- Replaced object string output with ingredients joined by newline
- Ensures each ingredient is printed on its own line as intended
…w controls

- Changed button IDs in HTML to 'auto-forward', 'auto-backward', and 'stop'
- Updated slideshow.js to use the new button IDs
- Fixed test errors caused by mismatched element IDs in automatic slideshow feature
…w controls

- Changed button IDs in HTML to 'auto-forward', 'auto-backward', and 'stop'
- Updated slideshow.js to use the new button IDs
- Fixed test errors caused by mismatched element IDs in automatic slideshow feature
- Uses that delay in milliseconds when auto-forward or auto-backward is activated.
- Disables/enables delay input along with auto buttons for a clean UX.
@HoussamLh HoussamLh added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Aug 6, 2025
@sambabib sambabib added Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Aug 9, 2025
<title>Title here</title>
<script defer src="slideshow.js"></script>
<title>Image carousel</title>
<link rel="stylesheet" href="style.css" />
</head>
<body>
Copy link

Choose a reason for hiding this comment

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

I ran your demo and I can see the buttons move up and down the page when the image changes to the next slide. if you had to improve the UX of this app, what would you change?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback! I would improve the UX by keeping the navigation buttons fixed in a consistent position, so they don’t shift when the image changes. I’d also add smoother transition effects between slides and ensure the carousel is responsive for different screen sizes.

Choose a reason for hiding this comment

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

this is an interesting approach but I am sure there is a better way to do this. Have you tried separating the image and control buttons on the page. Perhaps having different containers for them?

}

forwardBtn.addEventListener("click", () => {
Copy link

Choose a reason for hiding this comment

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

solid implementation here. well done!

Copy link
Author

Choose a reason for hiding this comment

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

Thank you! I appreciate the feedback.
I’ll keep refining the UX to make the carousel smoother and more consistent.

@@ -12,4 +12,4 @@ const address = {
postcode: "XYZ 123",
};

console.log(`My house number is ${address[0]}`);
console.log(`My house number is ${address[0]}`);

Choose a reason for hiding this comment

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

did you check your console to see the result of this console.log? You appear to be using a property reserved for arrays on an object. My log shows me "My house number is undefined" which is not the expected behaviour.

@@ -12,4 +12,4 @@ const recipe = {

console.log(`${recipe.title} serves ${recipe.serves}
ingredients:

Choose a reason for hiding this comment

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

You haven't solved the problem here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review in progress This review is currently being reviewed. This label will be replaced by "Reviewed" soon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants