react-scroll-sync icon indicating copy to clipboard operation
react-scroll-sync copied to clipboard

feat(usescrollsync): add scroll syncing via hook: `useScrollSync`

Open pcattori opened this issue 3 years ago • 12 comments

@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 replace horizontal=true and vertical=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.

pcattori avatar Mar 30 '21 17:03 pcattori

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>
  )
}

pcattori avatar Mar 30 '21 19:03 pcattori

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?

okonet avatar Apr 10 '21 08:04 okonet

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:

  1. Keep the components. Support both components and hooks.
  2. Same as (1), but deprecate the components. This will give us some time to get feedback for bugs/features for the hook implementation.
  3. Remove the components.

I think I lean towards (2). What do you think @okonet ?

pcattori avatar Apr 11 '21 02:04 pcattori

@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 avatar May 07 '21 06:05 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?

@okonet sounds good! I'll take a look at this over the weekend to finalize things.

pcattori avatar May 07 '21 12:05 pcattori

@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

pcattori avatar May 08 '21 01:05 pcattori

Looking great! I'd say, let's release it 🚢

okonet avatar May 11 '21 13:05 okonet

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

okonet avatar May 11 '21 13:05 okonet

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 avatar May 11 '21 13:05 okonet

@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>.

jayhuang-7 avatar May 24 '21 12:05 jayhuang-7

@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>.

@jayhuang-7 : could you provide an example where this happens?

pcattori avatar May 24 '21 13:05 pcattori

@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.

jayhuang-7 avatar Jun 01 '21 14:06 jayhuang-7