webui: devices view #4700

Merged
merged 2 commits into from Oct 16, 2016

Projects

None yet

7 participants

@crcro
Contributor
crcro commented Oct 2, 2016 edited

Please note

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

basic view:
screen shot 2016-10-08 at 10 13 55 pm

detail view:
screen shot 2016-10-11 at 8 28 05 pm

@crcro crcro changed the title from webui: devices view cleaned to webui: devices view Oct 2, 2016
@laf
Member
laf commented Oct 3, 2016

That's awesome work @crcro :)

I would say though I'm not a fan of the taller status boxes and prefer the old way ( @librenms/reviewers )?

image

@laf laf added WebUI Blocker labels Oct 3, 2016
@crcro
Contributor
crcro commented Oct 3, 2016

main reason was to keep things within same aspect ratio ...

On Oct 3, 2016, at 21:06, Neil Lathwood notifications@github.com wrote:

That's awesome work @crcro :)

I would say though I'm not a fan of the taller status boxes and prefer the old way ( @librenms/reviewers )?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@paulgear
Member
paulgear commented Oct 3, 2016 edited

I would rather keep the status boxes smaller and vertically centred the way they are now. As it stands, I would probably never use the one line version. I think it leaves out too much info.

If we're trying to make a useful one-line version, I would prefer to do it by reducing the space between the elements and separating out the columns (i.e. separate columns for ports & sensors, separate column for OS & version, etc.).

@laf
Member
laf commented Oct 4, 2016

Changing the number of columns between the basic / detail view would add unnecessary complexity to the code so sticking with the same # of columns across both would be my preferred way.

I can only imaging the basic view had more of a use when it wasn't a paginated page as it reduces the queries run. However I do think it looks good, it's a lot cleaner and simple. Gets my vote.

@crcro
Contributor
crcro commented Oct 4, 2016

what @paulgear said has made me thinking about adding more one-line info in basic view.
@laf ... changing the columns between view is not such a big complexity, but no pagination would add a possible pitfall for long lists of devices?

@laf
Member
laf commented Oct 4, 2016

It'll increase the complexity of the codebase for no real benefit imho.

Don't remove the pagination though

html/pages/devices.inc.php
+ } else {
+ $is_selected = '';
+ }
+ $graphs_types .= "<option value=" . generate_url($vars, array('format' => 'graph_' . $avail_type)) . " $is_selected >$display_type</option>";
@Jellyfrog
Jellyfrog Oct 5, 2016

missing quotes around value

@crcro
crcro Oct 8, 2016 Contributor

fixed in latest push

@laf laf Added column selection
d20d6c3
@laf

Aside from those changes, looks good.

html/pages/devices.inc.php
+ <script>
+ var grid = $("#devices").bootgrid({
+ ajax: true,
+ rowCount: [5, 10, 50, 100, 250, -1],
@laf
laf Oct 10, 2016 Member

This defaults to 5 now which is too small, 50 made more sense.

html/pages/devices.inc.php
+ rowCount: [5, 10, 50, 100, 250, -1],
+ columnSelection: false,
+ formatters: {
+ "status": function (column, row) {
@laf
laf Oct 10, 2016 Member

This is still showing a bigger box with small text - I think you just need to revert the status box back imho.

@laf
Member
laf commented Oct 13, 2016

This imho is still too low a number, it's now 10 devices per page and should be 50.

Also the text in the box is still too small.

@laf laf removed the Blocker label Oct 14, 2016
@laf
laf approved these changes Oct 14, 2016 View changes
@laf
Member
laf commented Oct 14, 2016

All good from my perspective.

@librenms/reviewers ?

@murrant

Looks good.

@murrant
Contributor
murrant commented Oct 15, 2016

Only thing I could think that could be improved is allow for user selectable columns. (check the all ports page for an example)

@crcro
Contributor
crcro commented Oct 16, 2016

@murrant besides vendor, metrics and actions columns all of them are sortable, do you need anything more, or maybe i didn't understood your request.

@crcro crcro fixes conflicts
ac0faab
@laf
Member
laf commented Oct 16, 2016

See my commit I just pushed.

@scrutinizer-notifier

The inspection completed: 3 new issues

@laf laf merged commit 5569884 into librenms:master Oct 16, 2016

1 of 2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@murrant
Contributor
murrant commented Oct 17, 2016

@crco Go to Ports, click the button on the far right, there are columns that aren't shown by default... But are able to be shown.

@crcro crcro deleted the crcro:webui-device-revamp branch Oct 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment