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 internal css and js #92

Closed
JohnAdib opened this issue Jun 15, 2018 · 15 comments
Closed

Use internal css and js #92

JohnAdib opened this issue Jun 15, 2018 · 15 comments

Comments

@JohnAdib
Copy link

Currently to load html output, linfo use 5 file contain 3 css, 1 js and 1 image as default and sometimes it being more for extra image or etc.

We can use internal css and js remove extra request. Someone maybe want to use linfo html preview as part of another service and they must customize location of media files. Because of that we can use internal css and js. For images i recommend to merge images into one file and convert it to base64 to use in css.

@JohnAdib
Copy link
Author

Also for change theme i think it's better to simply add a theme name as class to body and use one file for all themes, because it's very simple and don't need include each file with all of styles!

@Gummibeer
Copy link
Contributor

Most browsers will cache assets and are also faster to request 5 smaller files instead of one big.
Base64 decoded images are also a lot larger.
The next reason against is maintainability. And for themes - it's in no way better/faster to request one css file with all themes instead of a single, replaceable, one with the current theme.

And most ones who use this in another project will build customized HTML and just use the API of this package.

@jrgp
Copy link
Owner

jrgp commented Jun 15, 2018

We could make the images a sprite sheet and minify/combine the JS and CSS files down, but I'm not sure what the gain would be as the size of JS and CSS for linfo is miniscule compared to other projects. All of it together is just a few KB.

@Gummibeer
Copy link
Contributor

And as a last reason against inline: "all" projects with an enabled content security policy will fail cause enable inline-unsafe is like don't have a CSP.

@JohnAdib
Copy link
Author

All of image in icons folder is about 27.8kb and combine them and after compressing is maybe less than 10kb. for example compress logo.png is reduce about 65% of file size via compressor.io
Also all of css is about 6.1kb and combine theme colors in one files is less than 8kb and it's nothing!
At least you can use one css, one image at all.

Also namespace is not used correctly in php codes and need to be fix. for example for create new instase for Timer, must use backslash at start of class name like \Timer because of namespace conflict.

@jrgp
Copy link
Owner

jrgp commented Jun 16, 2018

This might be worth thinking about.

Regarding that namespace thing, does using it shorthand (eg Timer) matter when we have the relevant use statements at the top of the file?

@JohnAdib
Copy link
Author

No it's not about using shorthand.
For example to define new instance from simple datetime class, you must declare as new \DateTime instead of new DateTime (with backslash before name of class). or to use try catch you must define exception with backslash because it's global class. like catch (\Exception $e).

This lets PHP know that this call should be resolved from the global space instead of approaching it relatively. Without backslash it's relative and it's may not work with global namespace in some frameworks using global namespaces.
maybe this article about Namespacing in PHP help understand this problem.

@Gummibeer
Copy link
Contributor

As an example:

$t = new Timer('Load Averages');

In all other places it's used with the correct use \Linfo\Meta\Timer; statement https://github.com/jrgp/linfo/search?l=PHP&p=1&q=Timer

@jrgp
Copy link
Owner

jrgp commented Jun 19, 2018

I think that BSDCommon was the only place out of 30 or so files where the use \Linfo\Meta\Timer; was missing. I just added it. Anyone see any other places that have missing use statements?

@Gummibeer
Copy link
Contributor

@jrgp I would recommend to use PHP cs fixer in Travis to prevent things like this in future.
I'm not 100% sure if it detects missing use statements but it can detect unused ones

Related to the theme thing and so on it would be an idea to switch to less/sass and gulp/grunt/webpack to keep code simple and readable but compress everything during build.
It also could make theming a lot easier if I just have to adjust some variables.

@ameliabradley
Copy link
Collaborator

Someone maybe want to use linfo html preview as part of another service and they must customize location of media files. Because of that we can use internal css and js.

If this is an integration into another service, you might be better off using the Linfo JSON output and creating our own skin? It would give you a great deal more flexibility.

We can use internal css and js remove extra request.

Coming from a web development background, I see why you would make this suggestion. It is a very common SEO optimization for websites. But I don't feel like this applies in our use case.

Inline images and CSS improve first-load performance, but hurt subsequent loads. I'm willing to bet most Linfo page views are in the "subsequent" bucket.

For images i recommend to merge images into one file and convert it to base64 to use in css.

All of image in icons folder is about 27.8kb and combine them and after compressing is maybe less than 10kb.

Sprite sheets would not be an optimization. Vanilla Linfo generally shows three images. If we do the math, adding a generous 1460 bytes http request overhead:

  • logo.png - 1646 bytes (+1460 bytes http req overhead)
  • os_linux.gif - 586 bytes (+1460 bytes http req overhead)
  • distro_debian.gif - 638 bytes (+1460 bytes http req overhead)
    = 7250 bytes (7.3 kb)

7.3 kb is still less than the proposed 10 kb sprite sheet. That means that page load time would be slower with a sprite sheet, presumably defeating the purpose.

It doesn't appear as though either of these changes would necessarily improve Linfo, at least in its current state.

@JohnAdib
Copy link
Author

If this is an integration into another service, you might be better off using the Linfo JSON output and creating our own skin? It would give you a great deal more flexibility.

Good solution, but need more time.

We can use internal css and js remove extra request.

Coming from a web development background, I see why you would make this suggestion. It is a very common SEO optimization for websites. But I don't feel like this applies in our use case.

It was not just about this issue. If target is reduce size of request, we can combine css files into one file and use like now as external file. If we do the math, adding a generous 1460 bytes http request overhead:

  • theme_default.css - 3327 bytes (+1460 bytes http req overhead)
  • icons.css - 2136 bytes (+1460 bytes http req overhead)
  • mobile.css - 596 bytes (+1460 bytes http req overhead)
    = 10439 bytes (10.4 kb)
    10.4 kb is more than one combined css with about 5966 + 1460 = 7426 bytes (7.5 kb)! :))

Inline images and CSS improve first-load performance, but hurt subsequent loads. I'm willing to bet most Linfo page views are in the "subsequent" bucket.

It's correct. but it's only 7.5 kb not mb!

@JohnAdib
Copy link
Author

Sprite sheets would not be an optimization. Vanilla Linfo generally shows three images. If we do the math, adding a generous 1460 bytes http request overhead:

logo.png - 1646 bytes (+1460 bytes http req overhead)
os_linux.gif - 586 bytes (+1460 bytes http req overhead)
distro_debian.gif - 638 bytes (+1460 bytes http req overhead)
= 7250 bytes (7.3 kb)
7.3 kb is still less than the proposed 10 kb sprite sheet. That means that page load time would be slower with a sprite sheet, presumably defeating the purpose.

I do it and create one sprite image for icons. 10 kb was guess. real size is 4610 bytes (4.6 kb). it size of one file image file contain all of images:) check image size

@Gummibeer
Copy link
Contributor

So you add 2.26 KB and 185 lines to reduce the amount of requests by 2 for an administrativ application that will never get seen by any search-bot or something else that's not the admin!?
This whole "issue" is optimization to death at the wrong place.

I've no usage stats but so far I used this package the delivered html is only an example and never used in production.
If you want a simple plug'n'play tool to monitor your server you will end-up with Munin or something similar. This package is in php (who the hell wants to monitor a whole server in PHP to see simple numbers?) - so it's primary use-case is to get system-stats for dashboards or calculations in PHP. So in all cases you will need the API and not the delivered HTML.
(not agains you @jrgp ) but the delivered HTML is ugly as hell - so no one will use this in a productive environment included in any other Application without changes. And as standalone this is a pretty bad choice (like said above).

@jrgp
Copy link
Owner

jrgp commented Jun 26, 2018

Sorry for the late reply everyone.

@LeeBradley thanks for the info. I was hoping you'd reply :)

@Gummibeer I fully agree. The most practical use for Linfo is likely its JSON API, and people will likely use something else other than PHP with a webserver for gathering metrics, like collectd or something else. I still spin Linfo's web UI up on servers I run and use it to look at them periodically, but it's definitely a niche use case.

As for the UI, I started this when I was 16 and wanted to create a lighter, faster, simpler and more functional version of phpsysinfo, with led to a deliberately minimal and lightweight UI. If it wasn't for @LeeBradley's blue/white theme he added over 7 years ago Linfo likely wouldn't even be usable.

@JohnAdib JohnAdib closed this as completed Sep 6, 2018
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

No branches or pull requests

4 participants