clickable and draggable scrollbar

[jordan-arenstein]

clicking or dragging the scrollbar now moves the cursor to the correct location. Should deal with #251

[codecov]

Codecov Report

Merging #2431 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #2431   +/-   ##
======================================
  Coverage    38.3%   38.3%           
======================================
  Files         300     300           
  Lines       12540   12540           
  Branches     1651    1651           
======================================
  Hits         4804    4804           
  Misses       7481    7481           
  Partials      255     255

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cca5847...fda6982. Read the comment docs.

[akinsho]

@jordan-arenstein thanks for the PR 😍, one slightly cosmetic-ish thing could the functions in the render be methods on the class and the scrollBarTop be in state so that its more explicit that the value is being changed?

[jordan-arenstein]

Thanks for the review @Akin909. I'm new to Flux and React, so I'm not 100% on the changes. I've committed, let me know if it's okay 😄 👍

[akinsho]

@jordan-arenstein no worries the changes I'm suggesting are that setLine beginScroll and trackScroll can be used as methods for the react component rather than re-created every time the component re-renders which is currently the case e.g.

interface IState {
	scrollBarTop: number
}
export class BufferScrollBar extends React.PureComponent<IBufferScrollBarProps, IState>
	public state = {
		scrollBarTop: null,
	}
	componentDidMount(){
		// you might not need this but the idea is that event listeners can be
		// setup when the component is created
         document.addEventListener("mousemove", this.trackScroll, true)
         document.addEventListener("mouseup", this.endScroll, true)
	}
	
	componentWillUnmount() {
		// event listeners are tidied up when the component is removed
		document.removeEventListener("mousemove", this.trackScroll, true)
        document.removeEventListener("mouseup", this.endScroll, true)
	}

	// Above the render function
	public setLine = (y: number) => {
		const lineFraction = Math.min(Math.max((y - this.state.scrollBarTop) / this.props.height, 0), 1)
            const newLine = Math.ceil(
                editorManager.activeEditor.activeBuffer.lineCount * lineFraction,
            )
            editorManager.activeEditor.activeBuffer.setCursorPosition(newLine, 0)
	}

	public beginScroll = (e: React.MouseEvent<HTMLDivElement>) => {
            e.preventDefault()
            // offsetY is definitely on the scrollbar in the beginning of the click
            this.setState({ scrollBarTop: e.nativeEvent.clientY - e.nativeEvent.offsetY })
            setLine(e.nativeEvent.clientY)
            document.addEventListener("mousemove", trackScroll, true)
            document.addEventListener("mouseup", endScroll, true)
        }

Currently its implicit that the scrollBarTop bit of information is being changed, and is a dependency for other functions react aims to make state explicit and provides an api for changing which is setState by setting state the component updates correctly based on the new state and the state changes can be accessed inside of react lifeCycleMethods like componentDidUpdate.

Currently the functionality should be largely unchanged by switching to this but it gives us a good jumping of point for any future changes, lemme know if any of that doesn't make sense

[jordan-arenstein]

Thanks! I guess I was on the right track. I've pushed my changes over an hour ago, but they aren't showing here?? They're visible on my fork though. I'm trying to figure it out.

[akinsho]

@jordan-arenstein thanks so much for making the changes 👍 I'm wondering if the event handlers cant be set in componentDidMount and removed in willUnmount since atm they are created and removed all the time over and over as a user scrolls, so I think the above event handlers in the didMount and willMount would probably do the same job but will only create one set of handlers

also an alternative to explicitly binding functions in a react component (or es6 class) is using an arrow function so instead of trackScroll(){ -> trackScroll = (stuff) => {

[jordan-arenstein]

Can you toggle a handler on and off without destroying it?

[jordan-arenstein]

If not, I don't see any alternative: the event handler is created when the scroll begins and is removed when it ends.

[akinsho]

ahhh 😆 I didn't quite understand what those event listeners were doing are they only supposed to exist whilst a user is scrolling? in which case the mounting and unmounting wouldnt be the right solution 👍

[jordan-arenstein]

They allow the drag to continue outside of the scrollbar - I figured it was too narrow a target, so the scroll continues as the mouse moves anywhere on the screen

[jordan-arenstein]

I'm taking a look at the arrow functions now.

[jordan-arenstein]

Based on this MDN link this is not supported in arrow functions, and so I can't use state within them

[jordan-arenstein]

or do you mean using arrow functions instead of method.bind(this)?

[akinsho]

yes exactly so instead of

constructor(props: any) {
        super(props)
        this.state = { scrollBarTop: 0 }
        // allow scroll events to be added and removed as event handlers
        this.trackScroll = this.trackScroll.bind(this)

you can do away with the constructor and do

	public state = {
		scrollBarTop: 0
	}
	
	public endScroll = (e: MouseEvent) => {
	// add content here
	// essentially the this binding will be permanently set to this component so the
	// this cannot be reassigned aka it implicitly works the same as a call to .bind(this)
	}

[jordan-arenstein]

fantastic! thank you! I'm testing now, will commit soon after 😄

[jordan-arenstein]

I don't know what changed?? 🤷‍♂️ but it's up now

[bryphe]

Just tried this out! This is awesome @jordan-arenstein , thank you for implementing this! 💯

[akinsho]

@jordan-arenstein are you ready for this to be merged?

[jordan-arenstein]

@Akin909 full steam ahead 👍