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 more width for rows of graphs #2461

Closed
wants to merge 562 commits into from

Conversation

Alucardfh
Copy link

Hi,

This is an attempt to allow the php code to size the graphs according the available width and height.
It uses an ajax call to populate the width and height of the browser.

To determine the size of each graph I divide the width by the number of graph to be drawn minus an arbitrary number of pixel to account for both side margins.

I have also decided to make graphs take the whole row if there is less than 800px available.

This is related to issue #2410.

Without modification :
normalwidth

With modification :

usemorewidth

laf and others added 30 commits October 17, 2015 21:00
Removed ping + performance graphs and tab if skip ping check
Centralised the date selector for graphs for re-use
Detect vcsa as vmware instead of generic
since all those fields are defined as "NOT NULL" they must be set before inserting into mysql.
Interface port permissions table (ports_perms) requires 'access_level' field to be NOT NULL
…annot

defer until the end due to successive tests). Replicated EXTEND handling
into freebsd branch.
change default solver to hierarchicalRepulsion, some default presets …
Updated to use env in distro script
De-dupe checks for hostname when adding hosts
fix percent bar also for quota bills
fix: setting user port permissions fails
Conflicts:
	html/css/styles.css
@LibreNMS-CI
Copy link

Auto-Deploy finished, Test PR at http://2461.ci.librenms.org or https://2461.ci.librenms.org

@Alucardfh
Copy link
Author

Hum, does the rebase should look like this ?
I am not that confident with my git skills, I might have messed up.

@laf
Copy link
Member

laf commented Nov 23, 2015

:) Don't worry about the rebase for now. Let's test some more and see if it's all ok. We can fix the rebase later.

@Alucardfh
Copy link
Author

Ok.

I have noticed an issue with graphs that have a lot of text in their legends in resolutions between 800x600 and 1024x768.
The legends is not rendered properly and make graphs of different size.

But to solve this, it would probably require to dynamically scale the text font in the graphs legend.

@LibreNMS-CI
Copy link

Auto-Deploy finished, Test PR at http://2461.ci.librenms.org or https://2461.ci.librenms.org

@laf
Copy link
Member

laf commented Nov 24, 2015

If you just put a min height / width in at the value we use now then no one is worse off but the people with larger screens will gain.

@LibreNMS-CI
Copy link

Auto-Deploy finished, Test PR at http://2461.ci.librenms.org or https://2461.ci.librenms.org

@Alucardfh
Copy link
Author

I have added a check to set the minimum size of the graphs if the computation give a value that is smaller than the old value.

This effectively only apply on screens width between 800px and 1024px as I didn't changed having each graph on a line for screens with less than 800px width.

I also tried to understand better why some graphs act weirdly and mess up the layout :

rrdgraph use the width and height only for the canva and not for the total size of the image.
You can make it do it by using --full-size-mode but this doesn't scale the axis text and legend making some graph ridiculously small.

This is made difficult to overcome as the lazy loading doesn't set the height and width of the and thus let it use the space it needs.

I'll dig around to see if it would be possible/easy/not ugly to force the width of the image that has been lazy loaded.

This would solve all padding issue due to graphs having slightly different width.

@LibreNMS-CI
Copy link

Auto-Deploy finished, Test PR at http://2461.ci.librenms.org or https://2461.ci.librenms.org

@Alucardfh
Copy link
Author

Ok actually the lazy loading has been made to remove the height/width in the img tag.

I have modified the lazy loading to just remove the height tag, this way the width of each graph is the width expected and the graphs is not compressed if the legend is long.

This avoid having weird padding to accommodate the unexpected extra width of some graphs.
It also make the page loading smoother as the lazy load only change the height layout.

There is still a default size set to the old value if it tries to render a graphs with smaller value.

Feedback ?

@laf
Copy link
Member

laf commented Nov 25, 2015

I get the opposite now without the height. On graphs that looked ok as they are now, they become squashed looking as the height is calculated too big.

The default images are: 215w 100h. I can shrink my browser with this now to get 215w but the height is then 176h which then looks odd :(

@Alucardfh
Copy link
Author

Ok, I'll think about how to solve that.

But my initial feeling is that it would require conserving the 215/100 ratio or similar to avoid weird looking graphs in resolution were the width x height ratio is reversed.

@laf
Copy link
Member

laf commented Nov 26, 2015

Yes that sounds good. Thanks for tackling this :)

@LibreNMS-CI
Copy link

Auto-Deploy finished, Test PR at http://2461.ci.librenms.org or https://2461.ci.librenms.org

@Alucardfh
Copy link
Author

I have implemented a solution that makes the height always width/2.15.
This prevent having weird looking graphs.

I also changed the code to only fit two graphs per row on resolution with width between 700px and 1024px.
I made so as displaying one graph per line made them too large and displaying all of them made them too small.

I also hooked a refresh after the resize event has updated the new browser size.
Its a workaround that avoid having to compute the actual ratio change for each different graph according to the change in browser size.

@f0o
Copy link
Member

f0o commented Nov 26, 2015

It looks good for me even on tiny screens.

However: I dont want this commit-mess to be merged, if this is the final version I will rebase it for @Alucardfh and re-submit it if that's OK for you

@Alucardfh
Copy link
Author

I agree, this PR is not good to merged (I possibly tracked my error to a merge I did instead of a rebase).

The only thing that will probably need tuning at this point is the size of the tooltip on large and small screen, but that can be dealt with in another PR.

@f0o : As you wish, I can also put all my changes in another PR if needed.

@Alucardfh
Copy link
Author

OK I have made another PR #2510 which should be less painful to merge.

@Alucardfh Alucardfh closed this Nov 26, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Jan 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet