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

Updated icons to use Font Awesome #5468

Merged
merged 6 commits into from Jan 21, 2017

Conversation

Projects
None yet
7 participants
@InsaneSplash
Contributor

InsaneSplash commented Jan 16, 2017

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.

  • Have you signed the Contributors agreement - please do NOT submit a pull request unless you have (signing the agreement in the same pull request is fine). Your commit message for signing the agreement must appear as per the docs.
  • Have you followed our code guidelines?
@mention-bot

This comment has been minimized.

mention-bot commented Jan 16, 2017

Thank you for submitting a PR @InsaneSplash! We have found the following @crcro, @laf and @Rosiak based on the history of these files to review this PR.

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 16, 2017

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

@murrant

This comment has been minimized.

Member

murrant commented Jan 16, 2017

Like this overall, but some of the replacements are a little too generic. Like specifically memory->cog. I think it is ok to keep the sensor icons as regular images for now.

@@ -203,11 +203,11 @@
$col_port = '';
}
if ($port_count) {
$col_port = '<img src="images/icons/port.png" align="absmiddle"> ' . $port_count . '<br>';
$col_port = '<i class="fa fa-link" style="color:#0080FF"></i> ' . $port_count . '<br>';

This comment has been minimized.

@Rosiak

Rosiak Jan 16, 2017

Contributor

Not a fan of the color of this.

This comment has been minimized.

@InsaneSplash

InsaneSplash Jan 16, 2017

Contributor

Its easy enough to change... I tried to match the blue icon that was originally used.

}
if ($sensor_count) {
$col_port .= '<img src="images/icons/sensors.png" align="absmiddle"> ' . $sensor_count;
$col_port .= '<i class="fa fa-heartbeat" style="color:#0080FF"></i> ' . $sensor_count;

This comment has been minimized.

@Rosiak

Rosiak Jan 16, 2017

Contributor

Same goes here.

This comment has been minimized.

@InsaneSplash

InsaneSplash Jan 16, 2017

Contributor

Same as above.

@InsaneSplash

This comment has been minimized.

Contributor

InsaneSplash commented Jan 16, 2017

I tried to match the current icons that were used for fa in the menus etc, but I agree that it needs some refinement and standardisation. This is something I was going to look in to once I received feedback that this change was interesting enough to pursue.

@murrant

This comment has been minimized.

Member

murrant commented Jan 16, 2017

Honestly, we could do with some auditing of icons to make sure they are all consistent and meaningful. It might even require some icon creation.

@InsaneSplash

This comment has been minimized.

Contributor

InsaneSplash commented Jan 16, 2017

Agreed. Would you like me to give it a go and see how it turns out?

@murrant

This comment has been minimized.

Member

murrant commented Jan 16, 2017

Sure, but if I were you I'd break it up into manageable steps.

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 16, 2017

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

@InsaneSplash

This comment has been minimized.

Contributor

InsaneSplash commented Jan 16, 2017

This commit is going to need the latest version of Font Awesome (4.7.0).

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 16, 2017

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

@laf

This comment has been minimized.

Member

laf commented Jan 16, 2017

We used to use icons everywhere and have moved a lot to FA.

I'm not personally a fan of using images as icons, it's just extra bloat we don't need. I'm happy with a full switch to FA.

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 17, 2017

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

6 similar comments
@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 17, 2017

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

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 17, 2017

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

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 19, 2017

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

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 19, 2017

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

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 19, 2017

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

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 19, 2017

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

@laf

laf approved these changes Jan 19, 2017

@laf

This comment has been minimized.

Member

laf commented Jan 19, 2017

Looks good to me. @librenms/reviewers

@@ -6,12 +6,60 @@
}
if (count($sensors)) {
switch (strtolower($sensor_type)) {

This comment has been minimized.

@murrant

murrant Jan 19, 2017

Member

Would this be worth moving to a function, or will it only be used here?

@@ -844,6 +844,7 @@
// General GUI options
$config['gui']['network-map']['style'] = 'new';//old is also valid
$config['theme_icon_colour'] = 'black'; // Font Awesome icon colour customisation

This comment has been minimized.

@murrant

murrant Jan 19, 2017

Member

I think this should be moved to CSS, instead of being a setting in php.

@murrant

This comment has been minimized.

Member

murrant commented Jan 19, 2017

Overall, the I like the icon selection 👍 .

All of the icons on the device overview feel too small and should probably be changed to fa-lg

There are few spots where color could help out quite a bit.

  • The color of the icons on the device overview tabs feels off, I'm not sure if it should match the text or somethings else or if it is fine as is.
  • I would really like to seem some colors in the device actions (table view and device overview) to make it easier to identify individual actions.

Those things could be worked out later. But I do think the default color setting should be moved to css.

@InsaneSplash

This comment has been minimized.

Contributor

InsaneSplash commented Jan 19, 2017

@InsaneSplash

This comment has been minimized.

Contributor

InsaneSplash commented Jan 20, 2017

cool!
I'll do a version with the larger icons for you and move the icon colour to the css :)

Lets see what you think?

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 20, 2017

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

@murrant

Great!

@laf laf referenced this pull request Jan 20, 2017

Merged

webui: health menu #5508

2 of 2 tasks complete
@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 20, 2017

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

1 similar comment
@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 20, 2017

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

@murrant

This comment has been minimized.

Member

murrant commented Jan 20, 2017

@InsaneSplash grep -r png . :-D

@laf

laf approved these changes Jan 20, 2017

@InsaneSplash

This comment has been minimized.

Contributor

InsaneSplash commented Jan 21, 2017

eheh yeah, but some need little more investigating :)

Im done with this PR. Will review once this has been merged and create a new PR with any changes etc.

InsaneSplash and others added some commits Jan 16, 2017

# This is a combination of 17 commits.
# The first commit's message is:
I agree to the conditions of the Contributor Agreement contained in doc/General/Contributing.md.

# This is the 2nd commit message:

Fixed error in pseudowires.inc.php files

# This is the 3rd commit message:

Change the colour of the port icon

# This is the 4th commit message:

Revert "Fixed error in pseudowires.inc.php files"

This reverts commit f64cb07.

# This is the 5th commit message:

Fixed error in pseudowires.inc.php files

# This is the 6th commit message:

Revised the primary menu to try standardise icons and layout. Fixed icon size for countdown_timer icons.

# This is the 7th commit message:

Fixed formatting non-compliance testing

# This is the 8th commit message:

Updated additional icons and adjusted to meet the new standard as per menu bar

# This is the 9th commit message:

found a few more lurkers

# This is the 10th commit message:

webui: Update add/edit user page to use their instead of his #5457 (#5460)

# This is the 11th commit message:

newdevice: Add wifi clients for Deliberant DLB APC Button, DLB APC Button AF and DLB APC 2mi #5456

# This is the 12th commit message:

various small doc improvements (#5459)

# This is the 13th commit message:

newdevice: Added CPU support for Edge Core OS

# This is the 14th commit message:

newdevice: Added support for CTC Union devices (#5402)

* newdevice: Added support for CTC Union devices

* added logo

* added unit test

* added missing snmprec file

* replaced logo

* reverted logo

* and again :/

# This is the 15th commit message:

fix: validate suid is set for fping (#5474)

# This is the 16th commit message:

fix as reported by @Compizfox (#5473)

# This is the 17th commit message:

fix: Updated prestiage detection #5453 (#5470)
@laf

This comment has been minimized.

Member

laf commented Jan 21, 2017

I've squashed this down and rebased for you, contains the agreement message still which is the most important. Just waiting for checks to pass and will merge.

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 21, 2017

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

laf added some commits Jan 21, 2017

@LibreNMS-CI

This comment has been minimized.

LibreNMS-CI commented Jan 21, 2017

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

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Jan 21, 2017

The inspection completed: 645 Issues, 18 Patches

@laf laf merged commit 47397d0 into librenms:master Jan 21, 2017

2 checks passed

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

@InsaneSplash InsaneSplash deleted the InsaneSplash:issue-5463 branch Jan 26, 2017

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