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

webui: Improve Services Page Look #7628

Merged
merged 1 commit into from Dec 24, 2017

Conversation

Projects
None yet
6 participants
@Rosiak
Contributor

Rosiak commented Nov 5, 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.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

Before:
before_r
After:
after_r

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Nov 5, 2017

The inspection completed: No new issues

@laf

This comment has been minimized.

Member

laf commented Nov 5, 2017

I like the tidying up of things, however the status check boxes now go away from how the devices page is - imho we should remain consistent as much as possible throughout the webui.

@FTBZ

This comment has been minimized.

Contributor

FTBZ commented Nov 5, 2017

I've a lot of services, I think that a small refactor to save space will be nice. Now this is a long list to scroll. This PR is not affecting the space, only the readability?

@florianbeer

This comment has been minimized.

Contributor

florianbeer commented Nov 6, 2017

Actually, I don't really think this does anything to tidy up things or make it more readable. It only moves the status color from the service name background to a strip on the left side and shifts around the other fields (e.g. description to the right, why?). Having the edit/delete buttons on top of each other also looks a bit weird to me, to be honest. At least we should have a bit of padding in between. The width of the "Last Changed" column was intentionally chosen to accommodate everything in one line.

Here's a screenshot of how the current services page looks on my installation:
screen shot 2017-11-06 at 21 10 00

@FTBZ

This comment has been minimized.

Contributor

FTBZ commented Nov 7, 2017

Didn't notice that the description column was gone. This is important for me too like @florianbeer. This is where I add the detail about the test because sometimes you're using the same plugin for different tests. Like the http for checking https and certificat.

@Rosiak

This comment has been minimized.

Contributor

Rosiak commented Nov 7, 2017

I believe the screenshots I included was bad, as it was taken on a rather small screen which could cause some confusion. Also I did include the descr column, I just forgot to name it.

Please take a look:
screen shot 2017-11-07 at 08 06 07
screen shot 2017-11-07 at 08 05 37

@FTBZ

This comment has been minimized.

Contributor

FTBZ commented Nov 7, 2017

Nice shot! Can the green label be reduced to keeping it on one line and save space on the screen?

@laf

This comment has been minimized.

Member

laf commented Nov 7, 2017

I still don't think we should be moving the colour to the side, it's different from the devices page and we really should have some consistency.

@Rosiak

This comment has been minimized.

Contributor

Rosiak commented Nov 7, 2017

@laf
Perfectly understand that, I could do the same to the other pages if people think it's OK.
Otherwise I'll just close this PR.

@laf

This comment has been minimized.

Member

laf commented Nov 7, 2017

Well in all honesty and I don't know if this is the same still but that's how obse.... device page looks (or at least similar)

@Rosiak

This comment has been minimized.

Contributor

Rosiak commented Nov 7, 2017

Ok.
I dunno how obs.. looks, as I got the inspiration from Incinga2.
I guess if people are not up for it, I'll close it.

@laf

This comment has been minimized.

Member

laf commented Nov 7, 2017

It does look nicer, but we would need all tables that do it the other way to be updated.

@Rosiak

This comment has been minimized.

Contributor

Rosiak commented Nov 14, 2017

So, should I proceed with updating the other ones or just close this PR?

@laf

This comment has been minimized.

Member

laf commented Nov 15, 2017

I'm happy for the other ones to be updated. Just not happy about the disjointed look if we merged this one PR :)

@laf laf referenced this pull request Nov 19, 2017

Merged

webui: alerts table refresh #7765

1 of 1 task complete
@FTBZ

This comment has been minimized.

Contributor

FTBZ commented Nov 20, 2017

I think we should start somewhere, no? The padding can be reduce IMHO.

@laf

This comment has been minimized.

Member

laf commented Nov 20, 2017

imho no, if we merged the alerts PR we would have 3 totally separate looks for tables like that. Sometimes our webui is the worst part of the software because it's changed by many people to look how they want, we need some kind of standards / consistency.

@kkrumm1 kkrumm1 added the WebUI label Nov 29, 2017

@laf laf merged commit d11808a into librenms:master Dec 24, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@lock

This comment has been minimized.

lock bot commented May 16, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 16, 2018

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