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

performatic changes #8

Merged
merged 1 commit into from
Jun 11, 2014
Merged

performatic changes #8

merged 1 commit into from
Jun 11, 2014

Conversation

jrgtt
Copy link
Contributor

@jrgtt jrgtt commented Jun 10, 2014

I've made some changes in the main.js aiming for a better perfomance in the page load.

<script src="js/vendor/raphael-min.js"></script>
<script src="js/vendor/g.raphael-min.js"></script>
<script src="js/vendor/piechart.js"></script>
<script src="js/main.js"></script>

Choose a reason for hiding this comment

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

Would loading them with the async attribute work here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that would be the case, the page relies on the main.js to write most of it's content and each file must be loaded in order to work.

@samdoiron
Copy link
Owner

To what extent were the changes successful?

@jrgtt
Copy link
Contributor Author

jrgtt commented Jun 10, 2014

I didn't understand your question.

@samdoiron
Copy link
Owner

Do you have any numbers indicating an improvement? To what extent is it more efficient?

@jrgtt
Copy link
Contributor Author

jrgtt commented Jun 11, 2014

Not a serious one, it's hard to measure async methods performance, it would require a test library.

For the sake of my consciousness, I put a var time = new Date().getTime() before the "squares creating" block. After the loop was done I logged console.log(new Date().getTime() - time) the difference was around 500ms.

samdoiron added a commit that referenced this pull request Jun 11, 2014
@samdoiron samdoiron merged commit 29cb95d into samdoiron:master Jun 11, 2014
@samdoiron
Copy link
Owner

That sounds good, and the performance tweaks are reasonable. Thanks for the improvements!

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.

3 participants