laboratoriobridge / bold

https://bold.bridge.ufsc.br
MIT License
75 stars 9 forks source link

[PagedTable] Undefined reference inside useRovingTabIndex #806

Closed mathbalduino closed 1 year ago

mathbalduino commented 1 year ago

Describe the bug

The <PagedTable /> component uses a <Dropdown /> component to display the size options, at the bottom of the table: image

When trying to close the dropdown (clicking outside of it or just by selecting an item), it panics with the following errors: image image

It seems that the reference used by useRovingTabIndex() is not there (is null) when the useEffect() cleanup function runs. This is the exact line where the error happens: image

To Reproduce

This is a copy of my yarn.lock (I'm using version 1.0.0-beta.55, the latest one):

...
bold-ui@^1.0.0-beta.55:
  version "1.0.0-beta.55"
  resolved "https://registry.yarnpkg.com/bold-ui/-/bold-ui-1.0.0-beta.55.tgz#4e8c4e065b2fd6b0f37f5d196e32f5f72399a0e5"
  integrity sha512-s+Xuxkfsl4WaKjb++rA8+s2SjOISl0lSM0GDrNvEfSCJJXoJEsAV3bAjdTO+TjP30+js2+H/uMjdimRnciRqDA==
  dependencies:
    "@emotion/core" "10.0.28"
    "@popperjs/core" "2.4.2"
    downshift "5.4.5"
    emotion "10.0.27"
    focus-trap-react "6.0.0"
    match-sorter "4.1.0"
    react-dnd "11.1.3"
    react-dnd-html5-backend "11.1.3"
    react-dropzone "11.0.1"
    react-popper "2.2.4"
    react-text-mask "5.4.3"
    react-transition-group "4.4.1"
    text-mask-addons "3.8.0"
...

This is some code that can be used to reproduce the issue (just render it on the browser and open/close the referred dropdown):

<PagedTable<string> 
  rows={['TESTE TESTE']} 
  page={0} size={10} totalPages={5} totalElements={500} 
  onPageChange={() => void 0} onSizeChange={() => void 0} 
  columns={[{ header: 'Test', name: 'test', render: s => s, }]}
/>

I'm using Google Chrome Version 115.0.5790.170 (Official Build) (64-bit), on Linux: image

Additional context

I solved the error locally, just by adding a ? before trying to clean the listeners of the useRovingTabIndex() ref, that may be null (and it is, indeed). My final useRovingTabIndex() hook looks like this: return () => rootRef.current?.removeEventListener('keydown', handleKeyDown) Instead of: return () => rootRef.current.removeEventListener('keydown', handleKeyDown)

Didn't open a Pull Request because I don't know if it is fully safe to solve it this way Thanks!

lucasjoao commented 1 year ago

Nice catch @mathbalduino! Feel free to open a pull request with the fix, looks fine to me solve in this way.