Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move headers to scroll element, making scrolling smooth and stay in sync #695

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

msand
Copy link

@msand msand commented Dec 3, 2019

Scrolling in the current version causes the header to flicker, because it's not inside the scroll element, and the scroll events processed by js are much fewer than actual events. This pr moves the rendering inside the scroll element. Thus making all the content stay in sync and causes much less nausea when using it.

Overview of PR

Changes are relatively minor, but running the lint:fix command caused quite a bit of noise.
Perhaps a separate pr which runs the lint fix first would make it clearer what has changed.

Don't forget to update the CHANGELOG.md file with any changes that are in this PR

@msand
Copy link
Author

msand commented Dec 3, 2019

Rebased against branch with lint:fix pre run msand#1

@Ilaiwi
Copy link
Collaborator

Ilaiwi commented Dec 4, 2019

Hey @msand , great work! I am still to do a thorough review of this PR, but I looked at this really quickly. I really like the approach you took to solve the issue. I've tried it before but had no luck making it work.
Could you please elaborate on all the sticky props and styling? why is that there? and also how would you handle different header heights? it could be 60 but it also could be 600 via the height prop on the timeline headers

@msand
Copy link
Author

msand commented Dec 4, 2019

@Ilaiwi Thanks! The sticky prop is only for the header, such that it stays on top, alternatively scrolls away when moving vertically. The old css method doesn't work with the changes to the DOM, so I decided to add support for that using a prop instead. The position: sticky on the sidebars, is because using position: fixed moves it to the wrong place, unless we set a transform, perspective, or filter on the ancestor we want to position it relative to, sticky does what we want by default, by sticking to its nearest ancestor that has a "scrolling mechanism" / containing block.
https://developer.mozilla.org/en-US/docs/Web/CSS/position#fixed
https://developer.mozilla.org/en-US/docs/Web/CSS/position#sticky

Different header heights could be handled the same way as resizing is handled now, at least the height of the header changes much less often than the scroll offset, probably causes less flickering/visual changes than the previous unsynchronised scrolling. At least personally, where I use it now, I know the needed height for the header depending on various settings, and just pass the headerHeight to the timeline as a prop.

@msand
Copy link
Author

msand commented Dec 4, 2019

I've left the handleHeaderRef prop intact, so that can probably be used with a very similar pattern to this:

resize = (props = this.props) => {
const { width: containerWidth } = this.container.getBoundingClientRect()
let width = containerWidth - props.sidebarWidth - props.rightSidebarWidth
const canvasWidth = getCanvasWidth(width)
const {
dimensionItems,
height,
groupHeights,
groupTops
} = stackTimelineItems(
props.items,
props.groups,
canvasWidth,
this.state.canvasTimeStart,
this.state.canvasTimeEnd,
props.keys,
props.lineHeight,
props.itemHeightRatio,
props.stackItems,
this.state.draggingItem,
this.state.resizingItem,
this.state.dragTime,
this.state.resizingEdge,
this.state.resizeTime,
this.state.newGroupOrder
)
// this is needed by dragItem since it uses pageY from the drag events
// if this was in the context of the scrollElement, this would not be necessary
this.setState({
width,
dimensionItems,
height,
groupHeights,
groupTops
})
this.scrollComponent.scrollLeft = width
this.scrollHeaderRef.scrollLeft = width
}

@FridgeWalaby
Copy link

Thanks a lot @msand . This fixed my problem with the scrolling on osx Chrome.

Would be nice if this could be merged into the master.
Or is there a nice way to work with this fork and add the updates of the master ? What are the best practices when working with forked libraries and keeping them updated?

Thanks for a reply in advance.

@AlyonaPianykh
Copy link

Any chances that this fix will appear in build soon?

@hraboviyvadim
Copy link

Hi! Any news about when it possibly can be merged into master?

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.

5 participants