Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

move xorigin.js after css #14228

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

phapdinh
Copy link

@phapdinh phapdinh commented Apr 19, 2018

This improves page load performance as it moves the script tag for xorigin.js after css

@phapdinh phapdinh changed the title move xorigin.js to body and load async to improve page performance move xorigin.js after css Apr 19, 2018
@vickramdhawal
Copy link
Collaborator

Hi Phapdinh.
Thanks for the contribution.

Can you provide some data on the performance improvement gained with this change ?
Or is there is a perceived performance improvement in UI Rendering with this change ? Does brackets seem to load faster ? Can you establish this ?

@phapdinh
Copy link
Author

The change will cause a very slight performance improvement that is hard to measure. Moving the file down also helps with keeping the code organized and inline with the comments in the html.

Copy link

@saulotoledo saulotoledo left a comment

Choose a reason for hiding this comment

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

Minor comments to prevent us to start ignoring minor details in the project.

src/index.html Outdated Show resolved Hide resolved
src/index.html Outdated Show resolved Hide resolved
@saulotoledo
Copy link

Hi @phapdinh. I suggest to squash commits into a single one. Merging too many commits for a simple change will make it difficult to track commits and changes later in the target branch.

@phapdinh
Copy link
Author

@saulotoledo I have squashed the five commits into 1

Copy link

@saulotoledo saulotoledo left a comment

Choose a reason for hiding this comment

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

@phapdinh It is ok for me. I will keep the rest of the discussion with @vickramdhawal. :)

@saulotoledo
Copy link

@navch What do you think about this PR?

@navch
Copy link
Contributor

navch commented Sep 4, 2018

I am fine with this change. Just make sure this is not causing any problem. To confirm that we should run all unit tests and make sure there should not be any new error in the console because of this change.

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

4 participants