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

SpatialNavigationNode isFocusable change is not working. #169

Open
lifeisegg123 opened this issue Nov 22, 2024 · 2 comments
Open

SpatialNavigationNode isFocusable change is not working. #169

lifeisegg123 opened this issue Nov 22, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@lifeisegg123
Copy link

Describe the bug
The SpatialNavigationNode component does not function correctly when the isFocusable prop is changed dynamically.

To Reproduce

const [focusable, setFocusable] = useState(false);

return (
  <SpatialNavigationNode isFocusable={focusable}>
    {() => <SomeView />}
  </SpatialNavigationNode>
);

Expected behavior
The component should be able to receive focus when isFocusable is set to true and should not receive focus when it is false.

Screenshots
If applicable, add screenshots to help explain your problem.

Version and OS

  • Library version:4.0.1
  • React Native version: react-native-web 0.19.13
  • OS: web

Additional context
I found that adding isFocused as a dependency at this line resolves the issue. However, I'm uncertain if this is an appropriate solution.

@lifeisegg123 lifeisegg123 added the bug Something isn't working label Nov 22, 2024
@pierpo
Copy link
Member

pierpo commented Nov 23, 2024

Hi,

Good catch, I hadn't thought of this. It's very unexpected indeed.
The patch is dangerous and will break things if you have nested nodes. It works if you don't 😁 But I would strongly not recommend it. Why? Because it unregisters / re-registers the node when isFocusable will change. Un-registering the node will also un-register the children, but we have no way to know this on the children side, so we will just kill all the children nodes.

This piece of code is about adapting the LRUD lib - the core underneath the spatial navigation - to React, and unfortunately this part is not very flexible (hence the non standard useEffect where we ignored some dependencies).

The cleanest way would be to use setNodeFocusable https://github.com/bbc/lrud/blob/master/docs/usage.md#modifying-node-focusability in a useEffect.

This needs a bit of manual testing but that should be OK 😁

// in Node.tsx
useEffect(() => {
  spatialNavigator.setIsFocusable(id, isFocusable);
}, [spatialNavigator, isFocusable])

// in SpatialNavigator.ts

public setIsFocusable(nodeId: string, isFocusable: boolean) {
  this.lrud.setIsFocusable(nodeId, isFocusable);
}

@lifeisegg123
Copy link
Author

@pierpo
thanks. it worked for my case 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants