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

Avoid unnecessary getBoundingClientRect() calls #11

Merged
merged 2 commits into from Jun 15, 2017

Conversation

rattuscz
Copy link
Contributor

May seem like useless micro optimization, but just with this simple change my sizable graph redraw went from 6s to 3s.
Each call to getBoundingClientRect() causes forced layout, avoiding when possible can make big difference.

Details

Avoids getBoundingClientRect() call until actually needed - paths that don't need it will not call this function

May seem like useless micro optimization, but just with this simple change my sizable graph redraw went from 6s to 3s.
Each call to getBoundingClientRect() causes forced layout, avoiding when possible can make big difference.
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 07c9300 on rattuscz:patch-1 into ** on naver:master**.

@@ -217,8 +215,9 @@ extend(ChartInternal.prototype, {
}
// show labels regardless of the domain if value is null
if (d.value === null && !$$.config.axis_rotated) {
if (yPos < box.height) {
yPos = box.height;
let boxHeight = textElement.getBoundingClientRect().height;
Copy link
Member

Choose a reason for hiding this comment

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

It's okay for getXForText(), but for getYForText(), getBoundingClientRect() are called multiple times.
The height value would be gotten from the above if the execution condition is met.

How about defining an inner function, which get the height calling getBoundingClientRect() just once?

	getYForText(points, d, textElement) {
		const $$ = this;
		let yPos;
		let getHeight = () => {
			const height = textElement.getBoundingClientRect().height;

			getHeight = () => height;
			return height;
		};
		
		if ($$.config.axis_rotated) {
			yPos = (points[0][0] + points[2][0] + getHeight() * 0.6) / 2;
		} else {
			...
		}
		
		// show labels regardless of the domain if value is null
		if (d.value === null && !$$.config.axis_rotated) {
			if (yPos < getHeight()) {
				yPos = getHeight();
				
		..

@netil
Copy link
Member

netil commented Jun 14, 2017

@rattuscz nice work!

BTW, we have some commit format style guideline for the contribution.

@rattuscz
Copy link
Contributor Author

@netil for getYForText - the conditions on the ifs actually make multiple calls impossible ( if i checked correctly ) - there is no value for which the function is called twice even without any memoize function

  • $$.config.axis_rotated
  • (d.value < 0 || (d.value === 0 && !$$.hasPositiveValue))
  • d.value === null && !$$.config.axis_rotated

the only place was the last if that's why i've used boxHeight variable.

will check the contrib guideline and update, missed it :-)

@netil
Copy link
Member

netil commented Jun 14, 2017

@rattuscz yeah, you're right. The condition doesn't met to be called multiple times.
Plz, check commit format and linting. I'll merge PR If there's no issue. thanks.

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 6af4989 on rattuscz:patch-1 into ** on naver:master**.

@netil netil merged commit ce95ca2 into naver:master Jun 15, 2017
@netil
Copy link
Member

netil commented Jun 15, 2017

Thanks @rattuscz for your contribution!

@rattuscz rattuscz deleted the patch-1 branch June 15, 2017 07:28
@netil netil modified the milestone: 1.1.0 Jun 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants