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

[COH-18154] Enhancement/improve spatial navigation logic #463

Merged

Conversation

MartinBozhilov-coh
Copy link
Contributor

  • Self-reviewed and fixed any obvious mistakes, TODOs, removed debugging code, logs, etc
  • Updated package.json in related components, the library or the cli
  • Tested in a browser
  • Tested in Gameface

Remove any item from the list if not applicable by editing this message.

@MartinBozhilov-coh MartinBozhilov-coh changed the title Enhancement/improve spatial navigation logic [COH-18154] Enhancement/improve spatial navigation logic Oct 21, 2024
Copy link
Contributor

@alien1976 alien1976 left a comment

Choose a reason for hiding this comment

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

Please add hold action when spatial navigation registers the key actions inside registerKeyActions

keyboard.on({
                    keys: [key],
                    callback: `move-focus-${direction}`,
                    type: ['press', 'hold'],
                });

@@ -27,7 +27,7 @@ Initializes the spatial navigation.

#### navigatableElement

The navigatableElement can either be a string with a element selector
The navigatableElement can either be a string with an element selector
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The navigatableElement can either be a string with an element selector
The `navigatableElement` can either be a string with an element selector.

@@ -46,6 +46,16 @@ This will create different areas to separate the navigation. If you pass only a

<object data="{{<staticUrl "interaction-manager/SpatialNavigation/grid-elemens-areas.html">}}" width="1050" height="700"></object>

#### Overlap (optional)

The `init` method takes an optional second argument, `overlap`, which accepts a value between 0.01 and 1. The default value is 0.5 (50%). It specifies the percentage of acceptable overlap between the current element and a potential element for navigation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The `init` method takes an optional second argument, `overlap`, which accepts a value between 0.01 and 1. The default value is 0.5 (50%). It specifies the percentage of acceptable overlap between the current element and a potential element for navigation.
The `init` method takes an optional second argument, `overlap`, which accepts a value between 0.01 and 1. The default value is 0.5 (50%). It specifies the percentage of acceptable overlap between the current element and the next potential element for navigation.

The `init` method takes an optional second argument, `overlap`, which accepts a value between 0.01 and 1. The default value is 0.5 (50%). It specifies the percentage of acceptable overlap between the current element and a potential element for navigation.

If you want the elements to overlap perfectly (without any offset) in order to navigate between them, set the overlap value to 1 (100%).

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add an image explaining the overlap similar to the one you've send us in slack so the users visual representation of what you are talking about.

The `init` method takes an optional second argument, `overlap`, which accepts a value between 0.01 and 1. The default value is 0.5 (50%). It specifies the percentage of acceptable overlap between the current element and a potential element for navigation.

If you want the elements to overlap perfectly (without any offset) in order to navigate between them, set the overlap value to 1 (100%).

Copy link
Contributor

Choose a reason for hiding this comment

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

Document that if you set invalid value that is <0 or >1 it will not set it and instead will use the default value.

if (this.enabled) return;
this.enabled = true;

this.add(navigatableElements);
this.activeKeys = JSON.parse(JSON.stringify(defaultKeysState));
this.registerKeyActions();

if (overlap) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also check if the overlap argument is valid and fallback to default value if not.

0 and <1

Comment on lines 216 to 225
let nextFocusableElement;
if (currentAxisGroup.length > 0) {
nextFocusableElement = this.findNextElement(direction, currentAxisGroup, x, y);

if (!nextFocusableElement) {
nextFocusableElement = this.getClosestToEdge(direction, currentAxisGroup, { x, y });
}

nextFocusableElement.element.focus();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let nextFocusableElement;
if (currentAxisGroup.length > 0) {
nextFocusableElement = this.findNextElement(direction, currentAxisGroup, x, y);
if (!nextFocusableElement) {
nextFocusableElement = this.getClosestToEdge(direction, currentAxisGroup, { x, y });
}
nextFocusableElement.element.focus();
}
if (!currentAxisGroup.length) return;
const nextFocusableElement = this.findNextElement(direction, currentAxisGroup, x, y);
if (nextFocusableElement) return nextFocusableElement.element.focus();
this.getClosestToEdge(direction, currentAxisGroup, { x, y }).element.focus();

Comment on lines 234 to 256
filterGroupByCurrentAxis(direction, focusableGroup, currentElement) {
return focusableGroup.filter((element) => {
if (direction === 'left' || direction === 'right') {
const lowerBoundary = Math.min(currentElement.y + currentElement.height, element.y + element.height);
const topBoundary = Math.max(currentElement.y, element.y);

const verticalOverlap = Math.max(0, (lowerBoundary - topBoundary));
const minHeight = Math.min(currentElement.height, element.height);
const overlapPercentage = verticalOverlap / minHeight;

return overlapPercentage >= this.overlapPercentage;
} else {
const rightBoundary = Math.min(currentElement.x + currentElement.width, element.x + element.width);
const leftBoundary = Math.max(currentElement.x, element.x);

const horizontalOverlap = Math.max(0, rightBoundary - leftBoundary);
const minWidth = Math.min(currentElement.width, element.width);
const overlapPercentage = horizontalOverlap / minWidth;

return overlapPercentage >= this.overlapPercentage;
}
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe split it in that way to reduce the code in the filter method and make it more readable.

Suggested change
filterGroupByCurrentAxis(direction, focusableGroup, currentElement) {
return focusableGroup.filter((element) => {
if (direction === 'left' || direction === 'right') {
const lowerBoundary = Math.min(currentElement.y + currentElement.height, element.y + element.height);
const topBoundary = Math.max(currentElement.y, element.y);
const verticalOverlap = Math.max(0, (lowerBoundary - topBoundary));
const minHeight = Math.min(currentElement.height, element.height);
const overlapPercentage = verticalOverlap / minHeight;
return overlapPercentage >= this.overlapPercentage;
} else {
const rightBoundary = Math.min(currentElement.x + currentElement.width, element.x + element.width);
const leftBoundary = Math.max(currentElement.x, element.x);
const horizontalOverlap = Math.max(0, rightBoundary - leftBoundary);
const minWidth = Math.min(currentElement.width, element.width);
const overlapPercentage = horizontalOverlap / minWidth;
return overlapPercentage >= this.overlapPercentage;
}
});
}
isOverlappingX(currentElement, nextElement) {
const lowerBoundary = Math.min(currentElement.y + currentElement.height, nextElement.y + nextElement.height);
const topBoundary = Math.max(currentElement.y, nextElement.y);
const verticalOverlap = Math.max(0, (lowerBoundary - topBoundary));
const minHeight = Math.min(currentElement.height, nextElement.height);
const overlapPercentage = verticalOverlap / minHeight;
return overlapPercentage >= this.overlapPercentage;
}
isOverlappingY(currentElement, nextElement) {
const rightBoundary = Math.min(currentElement.x + currentElement.width, nextElement.x + nextElement.width);
const leftBoundary = Math.max(currentElement.x, nextElement.x);
const horizontalOverlap = Math.max(0, rightBoundary - leftBoundary);
const minWidth = Math.min(currentElement.width, nextElement.width);
const overlapPercentage = horizontalOverlap / minWidth;
return overlapPercentage >= this.overlapPercentage;
}
filterGroupByCurrentAxis(direction, focusableGroup, currentElement) {
return focusableGroup.filter((element) => {
if (direction === 'left' || direction === 'right') return isOverlappingX(currentElement, element);
return isOverlappingY(currentElement, element)
});
}

@@ -1,6 +1,6 @@
{
"name": "coherent-gameface-interaction-manager",
"version": "2.1.2",
"version": "2.1.3",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a feature so the version should be 2.2.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, true. Initially it was supposed to be a bug fix so I didn't consider it a feature but when I added the overlap I forgot to change the version. :D

@MartinBozhilov-coh MartinBozhilov-coh merged commit 9672e2d into master Oct 23, 2024
3 of 4 checks passed
@MartinBozhilov-coh MartinBozhilov-coh deleted the enhancement/improve-spatial-navigation-logic branch October 23, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants