search

NoriginMedia / Norigin-Spatial-Navigation

React Hooks based Spatial Navigation (Key & Remote Control Navigation) / Web Browsers, Smart TVs and Connected TVs
MIT License
315 stars 94 forks source link

Add focusOptions in conjunction with shouldFocusDOMNode #137

Closed cohenmax9 closed 1 month ago

cohenmax9 commented 2 months ago

Is your feature request related to a problem? Please describe. I need to leverage the preventScroll prop of HTMLElement focus method in order to disable the default scrolling behavior.

Describe the solution you'd like I am proposing adding a prop to init named domNodeFocusOptions in order to pass { preventScroll: true }. This prop will only be relevant in conjunction with shouldFocusDOMNode.

Example:

init({
   shouldFocusDOMNode: true,
   domNodeFocusOptions: {
       preventScroll: true
   }
})

Describe alternatives you've considered A clear and concise description of any alternative solutions or features you've considered.

Additional context I am handling the scrolling behavior manually in my app.

cohenmax9 commented 2 months ago

Hey @asgvard @predikament @Braggiouy! Apologies for being impatient, but I'd love to get some feedback on the above and see if you'd be willing to incorporate this change. I have a local branch where I have this built out, that I'd be happy to share. Thanks so much!

briannovak commented 2 months ago

@cohenmax9 - having a similar problem and think this could be useful - thanks for finding a potential fix and submitting it!

cohenmax9 commented 2 months ago

@xavi160 would you be able to help out with my above request? It sounds like I'm not the only one encountering this issue. I would be happy to push up my branch and create a PR, but I do not have permissions to do so. Thank you!

Braggiouy commented 2 months ago

@cohenmax9 Sorry for the delay in getting back to you. Most of our team is currently on leave. Could you please create a fork of the repository and make a PR from there? This way, we can see exactly what you are looking for and fully understand what is required.

cohenmax9 commented 2 months ago

Hi @Braggiouy! Thanks so much for your response! I went ahead and created a PR. Please let me know if you have any questions or suggestions. Thanks! https://github.com/NoriginMedia/Norigin-Spatial-Navigation/pull/139

cohenmax9 commented 1 month ago

Hi @asgvard @predikament @Braggiouy @xavi160!

I am wondering if there's anything else you need from me to get #139 merged. I'd love to see this change incorporated as it is blocking progress on my app development.

Thanks! Max

predikament commented 1 month ago

@cohenmax9 Hello!

Thanks for opening a PR with this change. Sorry for the slow response, a big part of the team has been on vacation.

The changes look relatively minimal, so I've pinged the team and we'll try to get this reviewed ASAP and get back to you.

Cheers

predikament commented 1 month ago

@cohenmax9 Your change has now been merged to main and as such this issue can be closed.

We'll be updating the docs for this and releasing a new version shortly.

I will edit this comment retroactively to point to the release containing the relevant changes.

Closing as completed - Thanks for the contributions! ❤️

Edit: Released in v2.1.1 - https://www.npmjs.com/package/@noriginmedia/norigin-spatial-navigation/v/2.1.1

cohenmax9 commented 1 month ago

Thank you all for your help with this!