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

feat(ui): show/hide node table columns #407

Merged
merged 6 commits into from
Feb 2, 2021
Merged

Conversation

ahochsteger
Copy link
Collaborator

This PR provides the possibility to show / hide individual table columns.

Button to show the selection menu:
image

Menu to choose the columns from:
image

The headers array allows to customize each column using the properties active (is currently visible or not) and selectable (is selectable in the menu or not).

Some not so important columns are now not shown by default (see screenshot above).
Since this reflects my personal opinion I'm interested in feedback, what the defaults should be.

It lays the foundation to provide much more data in the nodes table (e.g. battery level, ...) without overloading the number of columns.

@coveralls
Copy link

coveralls commented Feb 1, 2021

Pull Request Test Coverage Report for Build 530128304

  • 0 of 17 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.04%) to 21.383%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/components/nodes-table/nodes-table.js 0 17 0.0%
Totals Coverage Status
Change from base Build 530052053: -0.04%
Covered Lines: 2049
Relevant Lines: 9812

💛 - Coveralls

robertsLando
robertsLando previously approved these changes Feb 1, 2021
Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Easy clean and useful. Thanks :)

@robertsLando
Copy link
Member

@ahochsteger Feel free to squash and merge once ready

@robertsLando robertsLando changed the title feat: show/hide node table columns feat(ui): show/hide node table columns Feb 1, 2021
@ahochsteger
Copy link
Collaborator Author

@robertsLando thanks for your feedback!

Are you ok with the defaults I've chosen to not show certain columns by default?

@robertsLando
Copy link
Member

@ahochsteger I would keep all visible by default, and also I would store the shown in localstorage (like for the filters etc)

@ahochsteger
Copy link
Collaborator Author

@robertsLando the list of columns is now stored in local storage and the full list of headers is shown by default.
As a consequence I've simplified the code a lot (eliminated the selectable and active header attributes since they are not really requred).

@robertsLando
Copy link
Member

@ahochsteger In order to fix the UI problems I think we could set a max number of columns based on the screen size. What do you think about this?

@ahochsteger
Copy link
Collaborator Author

@ahochsteger In order to fix the UI problems I think we could set a max number of columns based on the screen size. What do you think about this?

@robertsLando I'm not sure, if the number of columns is a good solution here if you compare the width of the "ID" column with the "Product" column that shows for example something like "A real push button (switch) available in several colors." (which should be reduced in the device config anyway).

@robertsLando
Copy link
Member

@ahochsteger What other solution do you propose to fix the other UI problem?

@ahochsteger
Copy link
Collaborator Author

@ahochsteger What other solution do you propose to fix the other UI problem?

I'm not sure, which "other" UI problem you are referring to in this context?

Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@robertsLando robertsLando merged commit 56b0c1c into master Feb 2, 2021
@robertsLando robertsLando deleted the customize-table-columns branch February 2, 2021 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants