fix(carousel): improved tab page navigation #5035
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,9 @@ import ddsSettings from '@carbon/ibmdotcom-utilities/es/utilities/settings/setti | |
| import ifNonNull from 'carbon-web-components/es/globals/directives/if-non-null.js'; | ||
| import CaretLeft20 from 'carbon-web-components/es/icons/caret--left/20.js'; | ||
| import CaretRight20 from 'carbon-web-components/es/icons/caret--right/20.js'; | ||
| import HostListener from 'carbon-web-components/es/globals/decorators/host-listener'; | ||
| import HostListenerMixin from 'carbon-web-components/es/globals/mixins/host-listener'; | ||
| import DDSCard from '../card/card'; | ||
| import styles from './carousel.scss'; | ||
|
|
||
| const { prefix } = settings; | ||
|
|
@@ -26,7 +29,7 @@ const { stablePrefix: ddsPrefix } = ddsSettings; | |
| * @csspart next-button The button to go to the next page. | ||
| */ | ||
| @customElement(`${ddsPrefix}-carousel`) | ||
| class DDSCarousel extends HostListenerMixin(LitElement) { | ||
| /** | ||
| * The scrolling contents node. | ||
| */ | ||
|
|
@@ -111,6 +114,36 @@ class DDSCarousel extends LitElement { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Handles card focus throughout pages. | ||
| * | ||
| * @param event The event. | ||
| */ | ||
| @HostListener('shadowRoot:focusin') | ||
| // @ts-ignore: The decorator refers to this method but TS thinks this method is not referred to | ||
| private _handleFocus = async ({ target, relatedTarget }: FocusEvent) => { | ||
| const currentContains = target !== this && this.contains(target as DDSCard); | ||
| const oldNotContains = target !== this && !this.contains(relatedTarget as DDSCard); | ||
| const currentCardIndex = Array.from(this.children).indexOf(target as HTMLElement); | ||
|
|
||
| // keep current page if tabbing back into the carousel after previously moving pages | ||
| if (currentContains && oldNotContains && currentCardIndex === 0) { | ||
| (this.children[this.start] as HTMLElement).focus(); | ||
| return; | ||
| } | ||
|
|
||
| if (currentContains) { | ||
| // going forwards, change page depending on card index | ||
| if (currentCardIndex >= this.start + this.pageSize) { | ||
| this.start = currentCardIndex - (currentCardIndex % this.pageSize); | ||
|
There was a problem hiding this comment. Considering the following condition:
And user keeps hitting tab key to reach to the code here. What should happen? There was a problem hiding this comment. @asudo I see, wasn't aware the Now in the condition you provided |
||
|
|
||
| // going backwards, change page depending on card index | ||
| } else if (currentCardIndex < this.start) { | ||
| this.start = Math.max(currentCardIndex + 1 - this.pageSize, 0); | ||
|
There was a problem hiding this comment. Do you want to apply your improvement to "page forward" logic to the "page backward" logic here, too? There was a problem hiding this comment. Since we'd use the same code going in either direction, the previous logic runs in both directions whenever the focus is out of the current page. |
||
| } | ||
| } | ||
| }; | ||
|
|
||
| /** | ||
| * Handles `click` event on the next button. | ||
| */ | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asudoh Yes, we still need that logic to focus on the current starting card after having tabbed outside the carrousel. Without it, the focus will always go to the first card automatically, out of view. This will focus on the current page's starting card.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thank you for clarifying - I suggest preserving the "focused card index" in
_handleFocus(). It can be done by "old contains but current node contains" detection and preservingevent.relatedTarget. In this way we can setthis.startaccording to the index of such "previously focused card".