react-scroll-sync
react-scroll-sync copied to clipboard
feat(usescrollsync): add scroll syncing via hook: `useScrollSync`
@okonet : I took a shot at https://github.com/okonet/react-scroll-sync/issues/36#issuecomment-639553613 .
Right now I wrote this as a stand-alone, typed React hook. Ideally, if we agree on the hook, we could expose it as a feature via import {useScrollSync} from 'react-scroll-sync'
, but additionally the existing ScrollSync
and ScrollSyncPane
component could optionally be refactored to make use of useScrollSync
(perhaps via context API).
Looking for feedback to see if this is on the right path before continuing to polish this PR.
CodeSandbox Example : syncing scroll of the red header and the white body on horizontal axis.
Relates to #22, #36
Example usage:
const MyComponent: React.FC = () => {
const [thead, theadRef] = useElement()
const [tbody, tbodyRef] = useElement()
useScrollSync([thead, tbody], { axis: 'horizontal' })
return (
<>
<div ref={theadRef} role="rowgroup" style={{ overflowX: 'auto' }}>{...}</div>
<div ref={tbodyRef} role="rowgroup" style={{ overflow: 'auto' }}>{...}</div>
</>
)
}
Other changes:
-
axis
(typed as'horizontal' | 'vertical' | 'both'
) to replacehorizontal=true
andvertical=true
options. This allows user to only enable the axes they want, as opposed to having to disable the axes they don't want. - Removed concept of "groups"; instead just use multiple calls of
useScrollSync
, one for each group. - Included a
useElement
hook for convenience. It just copies the recommended approach from the React docs and wraps it in a hook.
Here's an example of a custom hook built on useScrollSync
:
import React from 'react'
import { useScrollSync } from 'react-scroll-sync'
export const useTableLayout = ({thead, tbody}) => {
useScrollSync([thead, tbody], { axis: 'horizontal' })
return {
thead: {
style: {
overflowX: 'auto',
overflowY: 'hidden',
/* https://www.w3schools.com/howto/howto_css_hide_scrollbars.asp */
scrollbarWidth: 'none',
},
},
tbody: {
style: {
overflow: 'auto',
},
},
} as const
}
import React from 'react'
import { useElement } from 'react-scroll-sync'
import { useTableLayout } from './useTableLayout'
const MyComponent = () => {
const [thead, tbodyRef] = useElement()
const [tbody, tbodyRef] = useElement()
const layout = useTableLayout({thead, tbody})
return (
<article>
<div role="table">
<div ref={theadRef} {...layout.thead} role="rowgroup">{...}</div>
<div ref={tbodyRef} {...layout.tbody} role="rowgroup">{...}</div>
</div>
</article>
)
}
I like this direction very much! Do you want to drop components in favor of hooks and do a breaking change? Or do you want to support both?
I like this direction very much! Do you want to drop components in favor of hooks and do a breaking change? Or do you want to support both?
I think it makes sense to keep the components, at least temporarily. Mostly because there may be use-cases that rely on features from the component implementation (e.g. I don't implement the onSync
callback yet).
Options:
- Keep the components. Support both components and hooks.
- Same as (1), but deprecate the components. This will give us some time to get feedback for bugs/features for the hook implementation.
- Remove the components.
I think I lean towards (2). What do you think @okonet ?
@pcattori I resolved almost all comments and awaiting for the final changes. Also, could you document the hook / update the documentation to reflect the new state?
I would say, let's remove components since maintenance efforts are way too high. Also documenting both approaches going to take longer. React isn't going back to pre-hook era anyways and we'll release this as a breaking change (we could even change the name of the project and package to reflect the change). WYT?
@pcattori I resolved almost all comments and awaiting for the final changes. Also, could you document the hook / update the documentation to reflect the new state?
I would say, let's remove components since maintenance efforts are way too high. Also documenting both approaches going to take longer. React isn't going back to pre-hook era anyways and we'll release this as a breaking change (we could even change the name of the project and package to reflect the change). WYT?
@okonet sounds good! I'll take a look at this over the weekend to finalize things.
@okonet: I've implemented the mode
parameter, added jsdoc for useScrollSync
, and reworked the example into a markdown usage guide.
I think we should be good to go with that! Let me know if we need anything else before merging.
Next steps (for future PRs):
- Remove the existing components to decrease maintenance overhead
Looking great! I'd say, let's release it 🚢
Oh one last thing, we should add it to the documentation website.
For that we'd need to modify the glob in the https://github.com/okonet/react-scroll-sync/blob/master/styleguide.config.js to include the hook as well. WYT?
You can preview it here https://deploy-preview-54--react-sync-scroll.netlify.app
So I'd suggest we change the glob to include the hook and also rename the example file to match the hook filename so it appears as a section in the styleguide.
@okonet, @pcattori Really appreciate your work, everyone.
but it seems like some type checking in TS are not passing. useElement
is returning CallbackRef<T | undefined>
but it is not compatible with RegacyRef<T | null>
.
@okonet, @pcattori Really appreciate your work, everyone. but it seems like some type checking in TS are not passing.
useElement
is returningCallbackRef<T | undefined>
but it is not compatible withRegacyRef<T | null>
.
@jayhuang-7 : could you provide an example where this happens?
@pcattori I just followed the Readme.md like this one.
const MyComponent: React.FC = () => {
const [thead, theadRef] = useElement()
const [tbody, tbodyRef] = useElement()
useScrollSync([thead, tbody], { axis: 'horizontal' })
return (
<>
<div ref={theadRef} role="rowgroup" style={{ overflowX: 'auto' }}>{...}</div>
<div ref={tbodyRef} role="rowgroup" style={{ overflow: 'auto' }}>{...}</div>
</>
)
}
However, it seems that we can't set theadRef
and tbodyRef
in line 7 and 8 due to type error that I indicated the above.