Skip to content

Conversation

@obadakhalili
Copy link

  • Possible addressed Issue: 805, and probably more.
  • Overview:
  1. Removes unnecessary onScroll event in Timeline.js.
  2. Handles scrolling properly in controlled mode.
  3. Synchs header and timeline.

2. Prevent the default behavior of onWheel event, and handle it customly. Which solves problems like: Header not synced with timeline, and unconsistent behaviors between different events, since different events are handled customly
const { traditionalZoom } = this.props


e.preventDefault()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is preventing the horizontal scroll from working. I think this can be fixed with

handleWheel = e => {
    const isHorizontalScrolling = e.wheelDeltaY === 0

    // zoom in the time dimension
    if (e.ctrlKey || e.metaKey || e.altKey) {
      e.preventDefault()
      const parentPosition = getParentPosition(e.currentTarget)
      const xPosition = e.clientX - parentPosition.x

      const speed = e.ctrlKey ? 10 : e.metaKey ? 3 : 1

      // convert vertical zoom to horiziontal
      this.props.onWheelZoom(speed, xPosition, e.deltaY)
    } else if (e.shiftKey) {
      e.preventDefault()
      // shift+scroll event from a touchpad has deltaY property populated; shift+scroll event from a mouse has deltaX
      this.props.onScroll(this.scrollComponent.scrollLeft + (e.deltaY || e.deltaX))
      // no modifier pressed? we prevented the default event, so scroll or zoom as needed
    } else if (isHorizontalScrolling) {
      e.preventDefault()
      this.props.onScroll(this.scrollComponent.scrollLeft + e.deltaX) 
      }
    }

  }

Also, this will cause performance issues, so you should consider adding window.requestAnimationFrame when calling this.props.onScroll(...)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @DarioG, please check the pr: #834

fixed this issue it was related to the another method

@obadakhalili obadakhalili reopened this Sep 15, 2021
@Ilaiwi Ilaiwi merged commit 6d20a29 into namespace-ee:master Oct 15, 2021
@DarioG
Copy link

DarioG commented Oct 15, 2021

This was just merged. Did you guys check that the vertical scrolling is still working even with these changes?

@Ilaiwi
Copy link
Collaborator

Ilaiwi commented Oct 15, 2021

@DarioG working on testing this out now then I will be merging it if all is working okay. (the merge that went though was by mistake I reverted it)
I am not sure about the issue you mentioned though. If you could explain a bit more? also, did the fix @StoikovOleh suggested work for you?

@Ilaiwi
Copy link
Collaborator

Ilaiwi commented Oct 15, 2021

@DarioG you are correct this PR would break horizontal scrolling. I tried @StoikovOleh and your suggested fix but both didn't work out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants