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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Theme optimisation #56

Closed
wants to merge 8 commits into from
Closed

Theme optimisation #56

wants to merge 8 commits into from

Conversation

jusuchin85
Copy link
Contributor

@jusuchin85 jusuchin85 commented Jan 22, 2018

The first commit (49cc807 is to address #45 by adding the LoadCSS script in default.hbs. Might not be the most optimized solution, but if there's a better way, let me know! 馃槃

The second commit is updating google-analytics.hbs to use the component from ga-lite (6823b88)

Please review.

@jusuchin85
Copy link
Contributor Author

Just as a heads up: the reason for ga-lite is to resolve Pagespeed, GTMetrix and Pingdom speed tests comments on Leverage browser caching.

The default GA script expires quickly (2 hours).

;c.parentNode.insertBefore(a,c)
})(window,document,"galite","script","https://cdn.jsdelivr.net/npm/ga-lite@2/dist/ga-lite.min.js");

galite('create', 'UA-57914210-1', 'auto');
Copy link
Owner

Choose a reason for hiding this comment

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

This google_analytics id is hard coded and we actually want to allow the consumer of this theme to be able to custom define/use their own id.

So can you make the following changes:

  1. Wrap this entire code change with the same if (window.ga_id) check that I made before.
  2. Replace galite('create', 'UA-57914210-1', 'auto'); with galite('create', 'UA-window.ga_id', 'auto');

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've updated the changes to the code so that a consumer can define their own GA code from the frontend. 馃槃

This has been pushed to my latest commit.

@kelyvin
Copy link
Owner

kelyvin commented Jan 24, 2018

Thanks for the pull request! I made one comment for the GA change.

Also for the loadcss issue to address #45, have you tested this extensively? Do you see a flash of 'unstyled' HTML before seeing the CSS load?

@jusuchin85
Copy link
Contributor Author

jusuchin85 commented Jan 26, 2018

I've pushed my changes for the GA change.

Also for the loadcss issue to address #45, have you tested this extensively? Do you see a flash of 'unstyled' HTML before seeing the CSS load?

Yes, I did. The page does flash with the unstyled HTML code for a few seconds until the CSS is fully loaded (you can view this on my site https://justinalex.com). The reason for this is because I was pushing for obtaining a high score with PageSpeed Insights.

However, if you believe that that is a hindrance, I'm more than happy for you not to merge that change (default.hbs). 馃槃

@jusuchin85
Copy link
Contributor Author

@kelyvin , found a way to fix the CSS flickering issue! 馃槃

The way I did was to extract any "above the fold" CSS styles using the Critical CSS generator, minified the CSS and added in the head> tag of default.hbs as an inline stylesheet.

The file still contains the LoadCSS script, which will handle the rest of the stylesheets.

I've pushed this as a latest commit as well.

@kelyvin
Copy link
Owner

kelyvin commented Jan 28, 2018

My only concern with your latest change is that if I decide to make any changes to the CSS, the "Critical css" code may need to be updated as well. This may become a maintenance issue as I am extremely forgetful and may forget to update the head tag of default.hbs when I make any css changes. I know myself too well 馃槄

So the alternative solution is to keep it as you had it and allow for the flash of unstyled HTML. However, although this would improve load time and allow a high score on the PageSpeed insights, I'm a firm believer that this leads to a disjointed user experience. Most of my non-technical peers even complain/ask me why there's the flash of unstyled text (FOUT) on some of their favorite websites. They're not fans. I'm willing to take the speed hit here just to provide a better "first time user" experience.

Because of these reasons, I don't think I can pull the LoadCSS changes to this codebase, but I'd be more than happy to pull in the GA changes.

Let me know what you think and thanks for contributing!

@jusuchin85
Copy link
Contributor Author

jusuchin85 commented Jan 28, 2018

Makes sense, @kelyvin . You're right, it's more on the user experience rather than all speed on PSI. Having a better user experience will drive potential visitors to your site rather than not. Plus, you also raise a valid point on maintenance (especially on maintenance for the hard-coded CSS on the default.hbs file).

I'll close this pull request, and create a new one with the GA optimisation fix. 馃槃

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