NoriginMedia / react-spatial-navigation

DEPRECATED. HOC-based Spatial Navigation. NEW Hooks version is available here: https://github.com/NoriginMedia/norigin-spatial-navigation
MIT License
226 stars 64 forks source link

withFocusable usage with forwardRef #110

Closed phillipcmittmann closed 2 years ago

phillipcmittmann commented 2 years ago

Describe the bug When attempting to use forwardRef with withFocusable, the compilation failed. Both withFocusable()(forwardRef(Thumbnail)) and forwardRef(withFocusable()(Thumbnail)) were tried.

To Reproduce

import React, { forwardRef } from 'react';

import { withFocusable } from '@noriginmedia/react-spatial-navigation';

import '../css/Thumbnail.css';

const Thumbnail = (props, thumbnailRef) => {
    return (
        <div
            className={'thumbnail'}
            ref={thumbnailRef}
        >
            <p>
                {props.index}
            </p>
        </div>
    )
};

export default withFocusable()(forwardRef(Thumbnail));
// OR export default forwardRef(withFocusable()(Thumbnail));

Expected behavior No idea to be honest.

Screenshots Captura de Tela 2021-11-29 às 10 01 10 Captura de Tela 2021-11-29 às 10 01 51

Additional context Theres only 3 components on the project itself, a Home screen, the playlists that render the Thumbnails. The project is being developed to Tizen, WebOS, Web and Mobile.

I have no experience with React, only React-Native. I tried to make the same project with ReNative, but because of the complexity of the tool and some limitations of the youtube player, I decided to migrate the project to React itself. When used with ReNative, all the focus was working as expected.

asgvard commented 2 years ago

Hi. I just tried to replicate it with:

const MyComponent = React.forwardRef((props, ref) => ...);
const MyFocuseableComponent = withFocusable()(MyComponent);

And haven't seen this issue. I'm not sure if moving the anonymous render function inside forwardRef is what makes a difference, but can you try this way as well? Otherwise we might need more information, like the exact versions of the dependencies, or a small repo where the issue can be reproduced.

phillipcmittmann commented 2 years ago

Hi. The code bellow resolved the issue. It was just a matter of how React works, it seems. Thanks for the help.

const MyComponent = React.forwardRef((props, ref) => ...);
const MyFocuseableComponent = withFocusable()(MyComponent);
chmiiller commented 2 years ago

hey @phillipcmittmann I see that you did exactly the same thing I'm trying to achieve: scroll to a withFocusable component with ref.current.scrollIntoView. I did like you did, but when I try to scroll to my component ref I get:

TypeError: null is not an object (evaluating 'item10ref.current.scrollIntoView')

That's my component's code:

import React from 'react';
import { withFocusable } from '@noriginmedia/react-spatial-navigation';

const Item = React.forwardRef((props, ref) => {
    return (
        <div ref={ref} style={{ backgroundColor: 'aqua', width: 300, height: 500 }} />
    );
});
Item.displayName = 'Item';

const FocusableItem = withFocusable()(Item);
export default FocusableItem;

and then on the parent I have:

const item1ref = React.useRef(null);
const item2ref = React.useRef(null);
const item3ref = React.useRef(null);

// when it's time to scroll:
const scrolling = () => {
  item3ref.current.scrollIntoView({ behavior: 'smooth' });
// This is when the crash happens
};

<div style={{ display: 'grid', rowGap: 5, gridTemplateColumns: 'repeat(2, 1fr)' }}>
    <Item ref={item1ref} />
    <Item ref={item2ref} />
    <Item ref={item3ref} />
</div>

Can you tell me how did you fix this problem and properly point to the Focusable component's ref?

phillipcmittmann commented 2 years ago

@chmiiller Change the Item component to receive a id as prop, so you can handle the focus using html instead. Something like this:

import React from 'react';
import { withFocusable } from '@noriginmedia/react-spatial-navigation';

const Item = (props) => {
    return (
        <div
            id={props.focusKey} // the props parameter can be anything, but the div needs a id
            style={{ backgroundColor: 'aqua', width: 300, height: 500 }}
        />
    )
}
Item.displayName = 'Item';

const FocusableItem = withFocusable()(Item);
export default FocusableItem;

And then on parent you will handle the focus like this:

const handleScrolling = (_, { focusKey }) => {
    const nodeThumb = document.getElementById(focusKey);
    nodeThumb.scrollIntoView(false);
}

<div style={{ display: 'grid', rowGap: 5, gridTemplateColumns: 'repeat(2, 1fr)' }}>
    <Item 
        focusKey={'item1'} // focusKey can be any other name, just remember to change in the child
        onBecameFocused={handleFocus}
    />

    <Item 
        focusKey={'item2'} // focusKey can be any other name, just remember to change in the child
        onBecameFocused={handleFocus}
    />

    <Item 
        focusKey={'item2'} // focusKey can be any other name, just remember to change in the child
        onBecameFocused={handleFocus}
    />
</div>

Notes: 1) I have no idea why I couldn't make a smooth transition, so I gave up on that for now

2) onBecameFocused callback can receive 3 props, the first is coordinates that Im not using; the second one is the props of the component, which in this case Im only using focusKey to find the node, so I had to extract that; and the third is a details object that was used when triggering the focus change, that Im also not using.

asgvard commented 2 years ago

It is not possible to pass "ref" into the underlying component if it is wrapped in HOC. Please refer to this documentation:

https://reactjs.org/docs/forwarding-refs.html#forwarding-refs-in-higher-order-components

asgvard commented 2 years ago

Also keep in mind that you get the node reference to the DOM element already as part of the layout returned by the onBecameFocused. So you might not even need forwardRef at all :)

chmiiller commented 2 years ago

thank you so much for your suggestion @phillipcmittmann ! Gonna try this out later today. @asgvard that's very useful, thanks for sharing it. the onBecameFocused also returns the X and Y positions, do you have any other suggestion to scroll the parent view to the focused element in a ReactJS project?

asgvard commented 2 years ago

I think setting "element.scrollLeft" or "element.scrollTop" with the style for "scroll-behavior: smooth" should work, but I haven't tried it for a long time as we mainly using react native web. However in some cases we used a translation with the custom transition styles. For example having a wrapper view as a scroll container with overflow: hidden, and an inner view with the actual scrollable content, you can modify the "translateX" or "translateY" based on where you want to scroll. Then setting the css transition to make it smooth.

chmiiller commented 2 years ago

thanks everyone, just tried a bit more today and just managed to make a smooth scroll to my items. @phillipcmittmann if you use node.scrollIntoView({ behavior: "smooth", block: 'center' }); it should scroll smoothly. (Chrome and TV - not on Safari) And thanks @asgvard , I'm using onBecameFocused's node parameter instead of getting the element from the DOM.

Here is a Code Sandbox in case someone else wants to implement this in the future: https://codesandbox.io/s/reactjs-spatial-navigationl-6vslv