Skip to content

fix(carousel): Prevent duplicate animation when navigating via keyboard #9848

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.

Sign up for GitHub

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

Merged
macandcheese merged 3 commits into from
Jul 25, 2024
Merged

fix(carousel): Prevent duplicate animation when navigating via keyboard #9848

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
56684ea
fix(carousel): Prevent duplicate animation when navigating via keyboard
macandcheese Jul 24, 2024
4d637f8
Merge branch 'dev' into macandcheese/9471-carousel-keyboard-animation
macandcheese Jul 25, 2024
5cf4c13
Merge branch 'dev' into macandcheese/9471-carousel-keyboard-animation
macandcheese Jul 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion packages/calcite-components/src/components/carousel/carousel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,8 @@ export class Carousel
return;
}

const lastItem = this.items.length - 1;

switch (event.key) {
case " ":
case "Enter":
Expand All @@ -512,13 +514,19 @@ export class Carousel
break;
case "Home":
event.preventDefault();
if (this.selectedIndex === 0) {
Copy link
Copy Markdown
Member

@driskull driskull Jul 24, 2024

Choose a reason for hiding this comment

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

should this move before event.preventDefault so event isn't prevented if nothing happens?

Copy link
Copy Markdown
Contributor Author

@macandcheese macandcheese Jul 24, 2024

Choose a reason for hiding this comment

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

I think I'd still expect it to be prevented. Without it, the Home / End keys could navigate the Carousel with one keystroke and then unexpectedly scroll the page with the next.

Copy link
Copy Markdown
Member

@driskull driskull Jul 24, 2024

Choose a reason for hiding this comment

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

Yep. makes sense.

return;
}
this.direction = "backward";
this.setSelectedItem(0, true);
break;
case "End":
event.preventDefault();
if (this.selectedIndex === lastItem) {
Copy link
Copy Markdown
Member

@driskull driskull Jul 24, 2024

Choose a reason for hiding this comment

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

should this move before event.preventDefault so event isn't prevented if nothing happens?

return;
}
this.direction = "forward";
this.setSelectedItem(this.items.length - 1, true);
this.setSelectedItem(lastItem, true);
break;
}
};
Expand Down