Skip to content
This repository was archived by the owner on Jun 27, 2020. It is now read-only.

Conversation

@nemesifier
Copy link
Member

This PR adds some general improvement to the work of @rohithasrk done in version 0.4.0 alpha.

I eliminated some redundancies, added some missing static images for the jquery ui datepicker, slightly improved the UI and other minor improvement.

This must be here in order for the <title> tag
in the detail.html template to work
jQuery is already available within django
- removed duplication of HTML
- removed duplication of JS logic
- reload graph directly with JS instead of generating it on the server
- removed redundant history views
- more consistent look between frontend & admin
- eliminated submit button in favor of change date event
Covering the date overlay for simplicity (no space left)
Having files like test_admin.py is preferable over admin.py
because admin.py may be confused with the model admin definitions
during development.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling e7e5555 on 0.4.0-improvements into c6b3cdc on master.

linkStrength: 0.1
});
// the graph may be loaded elsewhere than body
window.__njg_el__ = window.__njg_el__ || "body";
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is amazing! Didn't know we could do all these 😮

Copy link
Member Author

Choose a reason for hiding this comment

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

probably not optimal because not very easy to read, but a more elegant solution would require more time.
we basically reuse this piece of code to load and reload the graph in frontend, admin and when loading older snapshots, the difference is that in the admin we set window.__njg_el__ to the overlay, and when using snapshots we first fetch the data from the API and then pass the data in the graph directly to the library.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. I understand. I really liked how you made it reusable

Copy link
Contributor

@rohithasrk rohithasrk left a comment

Choose a reason for hiding this comment

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

This can be merged @nemesisdesign. I understood that there's a lot of JS to learn. 😄

@nemesifier nemesifier merged commit e7e5555 into master Aug 25, 2017
@rohithasrk rohithasrk deleted the 0.4.0-improvements branch December 27, 2017 07:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants