core-scroll: dispatch "scroll.change" (i.e. initialise buttons) when children change

[tolu]

whoever is working on core-components right now - THANKS, it is such a great bundle of things ❤️

Adding children to a <CoreScroll> after the component has rendered (because async) does not trigger the scroll.change event and subsequently (for jsx anyway) does not set the correct state for the scroll buttons (given the example implementation seen below).

Currently scroll.change is triggered on first render, when both scrollLeft and scrollLeft is 0 resulting in not displaying any scroll-buttons if the child list is empty. This PR adds a mutation observer that trigger this.handleEvent so that buttons can be re-initialised.

Another solution is of course to solve this from the outside using a ref. I do feel like this behavior (dispatching scroll.change) when children change is a better solution since it solves all use cases where children can be dynamically added or removed.

I'm happy to update the PR with all or any requested changes 🙌

class MyScroll extends React.Component {
    constructor (props) {
      super(props)
      this.state = { children: null }
      this.onScroll = this.onScroll.bind(this)
      setTimeout(() => {
        // Children set async -> state of disabled buttons are wrong (not updated)
        this.setState(s => ({...s, children: (
          <>
            <div>1</div><div>2</div><div>3</div><div>4</div><a href="#">5</a>
            <div>6</div><div>7</div><div>8</div><div>9</div><div>10</div>
            <div>11</div><div>12</div><div>13</div><div>14</div><div>15</div>
          </>
        )}))
      }, 500);
    }
    onScroll ({target}) {
      console.log('ON SCROLL');
      this.setState(s => ({
        ...s,
        left: target.scrollLeft ? () => target.scroll('left') : null,
        right: target.scrollRight ? () => target.scroll('right') : null
      }))
    }
    render () {
      return <div>
        <button disabled={!this.state.left} onClick={this.state.left}>Left JSX</button>
        <button disabled={!this.state.right} onClick={this.state.right}>Right JSX</button>
        <div className="my-wrap">
          <CoreScroll className="my-scroll" onScrollChange={this.onScroll}>
            {this.state.children}
          </CoreScroll>
        </div>
      </div>
    }
  }
  ReactDOM.render(<MyScroll />, document.getElementById('jsx-scroll'))

[skjalgepalg]

Thank you very much for the PR and your input on this!

I've added some suggestions to modify your implementation to be more in line with other core-components using MutationObserver in a similar manner. Give me a heads-up if you have questions.

Ref https://github.com/nrkno/core-components/issues/637 I suspect running npm run test may fail, unfortunately, but running npm run lint should give you some hints to conform to the standardjs codestyle used across the repo.

If you're up for making one, it would be awesome to have a test-case to cover this scenario.

[tolu]

thanks for the feedback, completely agree on all accounts 😅

I'll update, fix and add a test 👍

[tolu]

@skjalgepalg edits made, test added 🙏

[tolu]

I was a little fast on the trigger there I noticed (forgot to fix all suggestions) should be in order now. Thanks for all the good feedback 🙏

[tolu]

just let me know if there is anything else 🙏

[skjalgepalg]

Have given this some more thought and suggest we address the following:

• Triggering a scroll.change when DOM is manipulated feels misleading to the intention of that event.
  • We should rather emit a separate scroll.DOMChange (very open for input on the name) event when a MutationRecord is type childlist.
  • We should keep the existing trigger of scroll.change in onConnected, but further updates to DOM will trigger scroll.DOMChange
  • Adding scroll.DOMChange to customEvents in core-scroll.jsx gives us onScrollDOMChange for React use-cases.

• Examples in docs on static.nrk.no could use a touch-up, will add an example of DOM changes similar to what we use in core-toggle and move more work on that side to a separate issue.

  I have solutions to the above that I can (maybe?) commit to this PR branch, or possibly as a PR to your fork @tolu, what would be most convenient for you?

  Edit: Opted to just add to the PR branch to keep from fragmenting the discussion more than necessary. Hope that's ok

[tolu]

I can understand what you mean but the fact that scrollLeft and scrollRight do change when children are added/removed makes me think dispatching scroll.change is really not misleading by any stretch 🤔

Adding a separate event also adds complexity for consumers and so increases the chance to make mistakes...

It is of course up to you but I'd go for simplicity over correctness in this case

[skjalgepalg]

Very good point with the complexity part. Reverted the scroll.DOMChange -event, while keeping the handling to the onDOMChange function. Restored the test as well