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 bootstraps own img-responsive class for responsive graphs #2633

Merged
merged 3 commits into from Jan 6, 2016
Merged

Use bootstraps own img-responsive class for responsive graphs #2633

merged 3 commits into from Jan 6, 2016

Conversation

arjitc
Copy link
Contributor

@arjitc arjitc commented Dec 15, 2015

Old: http://i.imgur.com/ZKHurbZ.png
When img-responsive is used: http://i.imgur.com/R6sTtiV.png
Automatically resizes as well when I resize the window: http://i.imgur.com/G5QxZN7.png

@LibreNMS-CI
Copy link

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

@laf
Copy link
Member

laf commented Dec 18, 2015

Does this negate the need for #2537?

@arjitc
Copy link
Contributor Author

arjitc commented Dec 18, 2015

@laf Yup, does not need the $w and $h values either.

@LibreNMS-CI
Copy link

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

1 similar comment
@LibreNMS-CI
Copy link

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

@Alucardfh
Copy link

I just tested this PR and it is indeed working very well.

The only downside is that the bootstrap responsive code doesn't upscale image. Which anyway isn't something we want as it would blur graphs. But this means that graphs are still tiny on big resolutions.

So I also tested the changes in the PR on top of the one I have in PR #2537 which looks quite good to me and avoid my ugly workaround to reduce the blur on graphs.

Current master behavior (resolution 2560x1440):

no-img-responsive-noupscale

With this PR :

img-responsive-noupscale

With this PR + PR #2537 ;

img-responsive-upscale

@paulgear
Copy link
Member

paulgear commented Jan 1, 2016

I've tested this along with PR#2537 and it looks perfect on my browser. Big thanks, @arjitc, and 👍 to merge both this & #2537.

paulgear added a commit that referenced this pull request Jan 6, 2016
Use bootstraps own img-responsive class for responsive graphs
@paulgear paulgear merged commit 170423d into librenms:master Jan 6, 2016
@lock lock bot locked as resolved and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants