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

Use ResizeObserver to get chart size #56

Merged
merged 7 commits into from Nov 26, 2022

Conversation

dpschen
Copy link
Contributor

@dpschen dpschen commented Sep 10, 2022

I'm trying to improve the general performance of the library because I found out that dragging the bar elements drops the frame rate below 60fps. My initial naive attempt here is to remove operations that I think are expensive and hope that on the way the performance will get better.

As a side effect this pull request fixes some appearance issues when new bars get created: They now don't animate from the left side or the chart but scale in from the center of their final position (see videos below).


Before

vue-ganttastic_before-change.mp4

New bar animates in from left side of chart and transforms to final position.

After

vue-ganttastic_after-change.mp4

New bar appears on final position.


The implementation prevents the expensive use of multiple getBoundingClientRect.
Because we pass down the DOM reference we don't need to use Element.clostest.

To make things simpler I used useResizeObserver from vue-use. This shouldn't increase file size by much since it's really well tree-shakable. If you don't want this we could write our own small implementation / copy the vue-use function over.

I couldn't find out why, but this doesn't fully resolve the appearance issue: On the homepage of the docs (https://infectoone.github.io/vue-ganttastic/) the animation issue is still present after the fix.

This branch is based on the fix-lint-and-docs pull request. The diff can be seen here: dpschen/vue-ganttastic@feature/fix-lint-and-docs...dpschen:vue-ganttastic:feature/further-improvements


My intend is to use the same principle for the barElement so that we don't have to use getElementById in combination with the expensive offsetWidth (see: https://gist.github.com/paulirish/5d52fb081b3570c81e3a). Meaning having the current dimensions of the bar always available so that we only have to pass reference to them.

Further performance optimisation could be archived by throttling dragging events with requestAnimationFrame.

@dpschen dpschen force-pushed the feature/further-improvements branch from d45c5a4 to fa41dc4 Compare October 24, 2022 13:23
@dpschen dpschen marked this pull request as ready for review October 24, 2022 13:30
Copy link
Owner

@zunnzunn zunnzunn left a comment

Choose a reason for hiding this comment

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

A positive side effect of this is that the code became more readable since access to .valueand getBoundingClientRect() has been removed on many spots.
I don't mind that VueUse is now installed. It is a pretty much official Vue library and there is no need to reinvent the wheel and the increased bundle size is negligible thanks to tree-shaking. After testing this out locally, I will be merging this.

@zunnzunn zunnzunn merged commit 8ffd6f6 into zunnzunn:master Nov 26, 2022
@dpschen dpschen deleted the feature/further-improvements branch December 13, 2022 13:45
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.

None yet

2 participants