Skip to content
This repository has been archived by the owner on Dec 22, 2023. It is now read-only.

Removed getBoundingClientRect + Controlled Scroll support #110

Closed
wants to merge 3 commits into from
Closed

Removed getBoundingClientRect + Controlled Scroll support #110

wants to merge 3 commits into from

Conversation

MaartenBruggink
Copy link

Hi @thebuilder, first of all thanks for this great library!

In this pull request there are two changes that intertwine a bit.

1. I noticed a node.getBoundingClientRect() on every scroll tick, this could be quite heavy performance wise so I changed these to a combination of node.offsetTop and node.offsetHeight and the corresponding for the horizontal axis.

2. This changes was needed to make this library work with a controlled scroll. This can be quite useful for sliders or websites that feature a form of smooth scrolling.

In this fork are still some Typescript related issues. I wanted to discues the nature of these changes first before diving into Typescript more.

Please let me know what you think!

@thebuilder
Copy link
Owner

Hey @MaartenBruggink

I'm not sure I fully understand what the *Controlled scroll does. Can you explain this?
I'm aware that using getBoundingClientRect has a slight cost, but I haven't noticed in performance issues in implementations.

A problem with using offsetTop is that it doesn't account for the parent's offset. I think this could be a potential issue - depending on the nested level of the element. You'd have to do a recursive loop through all the parent elements, and by then it's properly better to just use getBoundingClientRect.

Another solution could be to throttle the the update - So it only fires something like every 50ms. This should be a value that can be controlled by the options.

@@ -42,9 +42,9 @@
"build:lib": "rollup -c",
"build:ts": "tsc -p tsconfig.build.json",
"build:copy": "node scripts/build-copy.js",
"dev": "concurrently -k -r 'jest --watch' 'yarn run storybook'",
Copy link
Owner

Choose a reason for hiding this comment

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

I'm using yarn for the project, so i would prefer if it wasn't changed to npm

@@ -6,7 +6,7 @@
"jsx": "react",
"moduleResolution": "node",
"noImplicitReturns": true,
"strict": true,
"strict": false,
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a reason for this change?

@MaartenBruggink
Copy link
Author

Hey @thebuilder

Controlled in the way that we own the scroll value in the React State. This way you could make horizontal slider work with a transform value instead of an element with a scrollbar.

I see the issue with the offsetTop, in the tests in StoryBook I didn't have any issues with offsetTop. I'll try to make a case where the parent has a transform value.

The change from yarn to npm should be changed back and the strict false is because of my limited knowledge in Typescript. I wanted to be able to discus these changes with you before cleaning up the Typescript interfaces.

@thebuilder thebuilder closed this Mar 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants