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

Add support for SVG images #5275

Merged
merged 4 commits into from Jan 8, 2017

Conversation

Projects
None yet
7 participants
@corny
Contributor

corny commented Dec 31, 2016

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

@laf

This comment has been minimized.

Member

laf commented Dec 31, 2016

Out of interest why?

I think a lot of this code will fail php 5.3 support due to the shorthand arrays used. Those will need updating.

Are you converting all os images? How would a new user provide an svg, it's easier to support png so we'd just end up with a mix.

@corny

This comment has been minimized.

Contributor

corny commented Dec 31, 2016

@laf the current PNG logos are looking aweful on high DPI displays.

I updated my pull request. foreach (array("svg", "png") as $ext) { should work with PHP 5.3? I replaced some logos by SVG versions. More work has to done, but PNG is still supported.

@laf

This comment has been minimized.

Member

laf commented Dec 31, 2016

I think I'd be more comfortable personally if you could add a doc on how to generate svg images to be used otherwise people are going to continue to use png images.

You've got more shorthand array calls which need updating. https://travis-ci.org/librenms/librenms/jobs/187973765

Thanks for contributing btw. You do need to sign the contributors agreement - you removed all of that text above, I've added it back in, please confirm as per the instructions.

@corny

This comment has been minimized.

Contributor

corny commented Dec 31, 2016

I added myself to the CONTRIBUTING.md, ok thanks for re-adding the text.

There is basically nothing to be aware of when creating SVG images. They are automatically scaled by stylesheets. I grabbed some images from wikipedia and I also cut some. Logos should just not be larger than ~ 20 kb. If the svg is too large, then paths should be simplified (e.g. with Inkscape). Where can I document it?

The SVG version of some logos is even smaller than the PNG.

@corny

This comment has been minimized.

Contributor

corny commented Jan 1, 2017

I also changed the second shorthand array and set a height for logo inside of the <span class="device_icon">.

@laf

This comment has been minimized.

Member

laf commented Jan 1, 2017

The contributors agreement needs to be signed as per the doc which means commiting a specific message.

As for docs, I'd say add something about the icon here:

http://docs.librenms.org/Developing/Support-New-OS/

and here:

http://docs.librenms.org/Support/FAQ/#how-do-i-add-support-for-a-new-os

@corny

This comment has been minimized.

Contributor

corny commented Jan 1, 2017

Thanks, I added a dedicated commit for the contributors agreement.

@laf

This comment has been minimized.

Member

laf commented Jan 2, 2017

Once you do some docs explaining to users creating new os' that svg is accepted and where to place the files then this will be good to merge imho.

@corny

This comment has been minimized.

Contributor

corny commented Jan 2, 2017

I added the docs.

@corny

This comment has been minimized.

Contributor

corny commented Jan 3, 2017

I used the SVG optimizer to shrink the images by more than 50 % in average.

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 3, 2017

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

@murrant

This comment has been minimized.

Member

murrant commented Jan 3, 2017

I would like to have SVGs for use in larger than 32x32 locations for v2, so +1.

The php code could use some refinement. I think the findImage() function needs to be removed and folded in. See code notes for more.

if (file_exists($config['html_dir'] . '/images/os/' . $device['os'] . '.png')) {
return $device['os'];
foreach ($possibilities as $icon) {
if (!empty($icon)) {

This comment has been minimized.

@murrant

murrant Jan 3, 2017

Member

You don't need this check if you use is_file() instead of file_exists() (which returns true for directories)

foreach ($possibilities as $icon) {
if (!empty($icon)) {
$name = findImage($icon);
if (!empty($name)) {

This comment has been minimized.

@murrant

murrant Jan 3, 2017

Member

If you bring the findImage code into this function, you won't need the !empty() check here...

@corny

This comment has been minimized.

Contributor

corny commented Jan 3, 2017

@murrant thanks for the review. I pushed another commit.

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 3, 2017

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

@murrant

This comment has been minimized.

Member

murrant commented Jan 3, 2017

Looking good now.

I would prefer the red and blue for the Cisco logo however :)

@corny

This comment has been minimized.

Contributor

corny commented Jan 3, 2017

@murrant The red/blue Cisco logo looks much better. I replaced it.

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 3, 2017

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

@murrant

This comment has been minimized.

Member

murrant commented Jan 3, 2017

@corny This needs a rebase (docs got moved around in another commit)

@librenms/reviewers Are we concerned about supporting IE8 and older? SVG img tags is IE9+ (and any other browser)

@laf

This comment has been minimized.

Member

laf commented Jan 3, 2017

IE 8 is completely end of life.

I'd be happy to drop support.

@corny

This comment has been minimized.

Contributor

corny commented Jan 3, 2017

I rebased my branch on top of master.

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 3, 2017

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

@murrant

This comment has been minimized.

Member

murrant commented Jan 3, 2017

Ok, I also reviewed the icons. They all look great, a few deviate in a way that might be concerning.

  • apple - some might not like the change to the rainbow logo, I'm fine with it.
  • aruba and mimosa: these could be changed to just the first letter of the logo as they were before.

A good indicator to me is the favicon of a companies website. Perhaps it would be a good idea to have both icon and logo, but it would be pointless if they are not used. That would probably be out of scope for this PR, so let's not worry about it ;)

@corny

This comment has been minimized.

Contributor

corny commented Jan 3, 2017

Yeah, I could not find a nice looking apple logo. Do you prefer another one?
https://commons.wikimedia.org/wiki/File:Apple_logo_black.svg (look at "Other versions")

I can cut the Aruba and Mimosa logos to the first letter. As a small square icon they would be definitely easier to be recognized. Other opinions?

@murrant

This comment has been minimized.

Member

murrant commented Jan 3, 2017

I prefer the rainbow logo over the plain black.

@corny

This comment has been minimized.

Contributor

corny commented Jan 4, 2017

I rebased my branch on top of master.

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 4, 2017

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

@laf

This comment has been minimized.

Member

laf commented Jan 6, 2017

trigger ci

@FTBZ

This comment has been minimized.

Contributor

FTBZ commented Jan 7, 2017

IMO, I don't think that you can use the logo that you prefer. You need to use the correct corporate logo that match the actual design. The Apple rainbow logo isn't used since 1998...

@laf

laf approved these changes Jan 8, 2017

@laf

This comment has been minimized.

Member

laf commented Jan 8, 2017

@librenms/reviewers I'm ok with this. Just rebasing it now so it's ready for a merge.

@f0o

f0o approved these changes Jan 8, 2017

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 8, 2017

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

@murrant

This comment has been minimized.

Member

murrant commented Jan 8, 2017

Did we want to change the Apple icon as FTBZ mentioned? It would probably be to the monochrome grey one.

@laf

This comment has been minimized.

Member

laf commented Jan 8, 2017

I'm not overly fussed but the black one is more modern imho.

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 8, 2017

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

@corny

This comment has been minimized.

Contributor

corny commented Jan 8, 2017

I replaced by Apple logo by the grey one and rebased again.

@murrant

murrant approved these changes Jan 8, 2017

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 8, 2017

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

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Jan 8, 2017

The inspection completed: No new issues

@murrant murrant merged commit 5ad8fd3 into librenms:master Jan 8, 2017

2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@corny corny deleted the digineo:svg-images branch Jan 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment