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

Button for exporting tables as csv #429

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Button for exporting tables as csv #429

wants to merge 12 commits into from

Conversation

emlun007
Copy link
Contributor

@emlun007 emlun007 commented Oct 19, 2017

Changes are made in layout.html and button libraries were added:

<script type="text/javascript" src="https://cdnjs.cloudflare.com/ajax/libs/pdfmake/0.1.32/pdfmake.min.js"></script> 
<script type="text/javascript" src="https://cdnjs.cloudflare.com/ajax/libs/pdfmake/0.1.32/vfs_fonts.js"></script>
<script type="text/javascript" src="https://cdn.datatables.net/v/dt/jszip-2.5.0/dt-1.10.16/b-1.4.2/b-html5-1.4.2/b-print-1.4.2/datatables.min.js"></script> 

@larsnaesbye
Copy link
Contributor

@mterzo : Have you taken a look at this and can you comment on what is needed for it to be merged.

@mterzo
Copy link
Contributor

mterzo commented Oct 30, 2017

CI is failing on the comment I made earlier. There are a bunch of JavaScript files added for local mode but I don’t see those files in the commits.

There was a major refactor of moving all the JavaScript to cacheable files instead of having all the JavaScript in HEAD of the html. We need make sure we don’t regress there.

@emlun007
Copy link
Contributor Author

emlun007 commented Nov 6, 2017

@mterzo , The javascripts that I added were for another commit where I am trying to solve the issue with the table sorting, so I will just remove the parts of the code that are not related to save button

@@ -45,8 +47,9 @@
<script src="{{ url_for('static', filename='js/scroll.top.js') }}"></script>
<script src="{{url_for('static', filename='js/d3.min.js')}}"></script>
<script src="{{url_for('static', filename='js/c3.min.js')}}"></script>
<script src="{{url_for('static',
filename='jquery-tablesort-v.0.0.11/jquery.tablesort.min.js')}}"></script>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lines 48 and 50 are identical. No changes

@emlun007 emlun007 changed the title WIP: button for exporting tables as csv Button for exporting tables as csv Nov 7, 2017
Copy link
Contributor

@kirkins kirkins left a comment

Choose a reason for hiding this comment

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

I didn't test this code but here are a few NITs I'm seeing off the bat.

@@ -2,7 +2,9 @@
<html lang="en">
<head>
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<title>{{config.PAGE_TITLE}}</title>
<title>Puppetboard</title>
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't change this line in the commit

@@ -2,7 +2,9 @@
<html lang="en">
<head>
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<title>{{config.PAGE_TITLE}}</title>
<title>Puppetboard</title>
<button id="export" style="clear: right;width:127px;height:50px;color:#0a214a;box-shadow: 4px 4px #0a214a;margin-top: 5 px; margin-left: 5 px">Export data as csv</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

You should move the inline style to the CSS block so it stays organized.

var titles = [];
var data = [];

$('.dataTable th').each(function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Moving my comment about indentation, as this where you stopped indenting.

If line 67 is indented and no block has ended this should also be indented.

Also another small thing lines 66 and 67 should have a 2 space indent as children of the function starting on 65.

@coveralls
Copy link

coveralls commented Dec 8, 2017

Coverage Status

Coverage remained the same at 81.76% when pulling ca7d238 on emlun007:master into c52f744 on voxpupuli:master.

@emlun007 emlun007 mentioned this pull request Dec 21, 2017
@coveralls
Copy link

coveralls commented Dec 21, 2017

Coverage Status

Coverage remained the same at 81.76% when pulling 868d11f on emlun007:master into c52f744 on voxpupuli:master.

@kirkins
Copy link
Contributor

kirkins commented Dec 21, 2017

I see the button and when I press it a file downloads.

But the file is empty. I don't see any errors in the javascript console log.

Does the csv contain data on your setup?

@kirkins
Copy link
Contributor

kirkins commented Dec 21, 2017

Other thoughts

  1. Remove the favicon, it's cute but not related to this PR.
  2. Nice job with the drop shadow but you will have to fix the design on this. I recommend integrating the button into the existing navigation bar.

Here is a preview of the button for those who haven't run the code locally:

untitled

@emlun007
Copy link
Contributor Author

The _macros.html is a different PR, same for favicon, I have no idea how the files got in this PR. The only change is in layout.html.

@emlun007
Copy link
Contributor Author

@kirkins , I updated layout.html with the version I am using. It works for chrome

@coveralls
Copy link

coveralls commented Dec 21, 2017

Coverage Status

Coverage remained the same at 81.76% when pulling 4c57a22 on emlun007:master into c52f744 on voxpupuli:master.

@kirkins
Copy link
Contributor

kirkins commented Dec 21, 2017

@emlun007 common mistake. You're using your master branch for this PR. That means the PR will reflect the state of your master branch.

Currently that branch has changes for multiple intended PRs.

You'll want to make a separate branch for each PR.

var titles = [];
var data = [];

$('.dataTable th').each(function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

The table doesn't have this class name on any of the pages I'm looking at.

What page did you test this on?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I see it works perfect on the Inventory page.

Do you only intend to use it with inventory? Or do you want it to work on other pages too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it works for all pages. I have tested on all facts, and it works there too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the browser? I have tested with IE and sometimes it doesn't work there

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to work on all fact pages for me too.

It doesn't work on Overview, Nodes, Facts, Reports, Metrics, Catalogs (kind of works), or Query.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because the data in Nodes,Facts ( try opening a fact, e.g. "ip adresses" there it works), reports is not initialized from dataTable, so there are no headers and data cells. So this code will only work on any data from dataTable

@coveralls
Copy link

Coverage Status

Coverage remained the same at 81.76% when pulling fd32dca on emlun007:master into c52f744 on voxpupuli:master.

@coveralls
Copy link

coveralls commented Dec 21, 2017

Coverage Status

Coverage remained the same at 81.76% when pulling f9edc83 on emlun007:master into c52f744 on voxpupuli:master.

@kirkins
Copy link
Contributor

kirkins commented Dec 21, 2017

For style a better option might be just adding a CSV button to the menu:

untitled

<a id="export_data_button" class="item" href="#">CSV</a>

Then maybe add some logic to hide the button on pages it doesn't work with.

@emlun007
Copy link
Contributor Author

The thing is, I have many options now, like CSV, Excel, PDF, Copy, Print so CSV button is just one of the options.

I was thinking of disabling button if the cells are empty, so the button will be gray and the pointer changes to a cross. It is a nice feature, just takes time to write code

@kirkins
Copy link
Contributor

kirkins commented Dec 21, 2017

I agree it is a nice feature. I don't think it can be released as is because people will see it on the first page, try, and think it's not working.

Also follow up on my menu suggestion. It might be a bit tricky because currently the menu is generated with:


      {%- for endpoint, caption in [
      ('index', 'Overview'),
      ('nodes', 'Nodes'),
      ('facts', 'Facts'),
      ('reports', 'Reports'),
      ('metrics', 'Metrics'),
      ('inventory', 'Inventory'),
      ('catalogs', 'Catalogs'),
      ('radiator', 'Radiator'),
      ('query', 'Query'),
] %}

Which doesn't seem to be able to take a value like ('#', 'CSV'), and has no room for the custom id.

@emlun007
Copy link
Contributor Author

I have tried to modify default_settings.py that defines the menu, but if you use anything else than 'fact name', 'fact', it gives 505 error so I don't think you can change the menu structure.

The button should work on the all th and td rows. The table uses .dataTable th and .dataTable td as input and it might be the case for facts, but I am unsure how nodes table is populated. I'm not too good with template engines so any help is appreciated.

@gdubicki
Copy link
Member

Are you interested in getting back to this PR, rebasing and making it work @emlun007?

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

Successfully merging this pull request may close these issues.

6 participants