-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Isolated dashboard
app JavaScript
#2155
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
base: main
Are you sure you want to change the base?
Conversation
{% block body_extra %}{% endblock body_extra %} | ||
<script src="{% static "js/djangoproject.js" %}"></script> | ||
|
||
{% block javascript %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The addition of this block is probably the most controversial change in this PR.
@@ -1,14 +1,19 @@ | |||
{% extends "base_dashboard.html" %} | |||
{% load i18n %} | |||
{% load static %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not load it alongside i18n
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I can do that. Just a personal style thing.
Updated in 9e53eab.
<script src="{% static "js/lib/jquery.flot.min.js" %}"></script> | ||
<script src="{% static "js/dashboard/utils.js" %}"></script> | ||
|
||
{% block dashboard_javascript %}{% endblock %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be simpler to not have dashboard_javascript
(and arguably it's already dashboard JS as we're in dashboard templates). Then when more JS is needed, use the super...
{% block javascript %}
{{ block.super }}
<script src="{% static "js/dashboard/index.js" %}"></script>
{% endblock %}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good thinking!
Updated in 7ac24bc.
This patch moves the `dashboard` app's JavaScript inclusion from the project's `require.js` structure to the app itself. This was achieved with a new block in the base template called `javascript`. The rest of the site gets the default JavaScript includes, and the `dashboard` app overrides the block to include the JavaScript specific to it. The `dashboard` app benefits by not knowing about JavaScript related to the rest of the site and vice versa. None of the actual `dashboard` JavaScript code was changed. It was only adapted to the new structure. - Added `javascript` block to base template - Added `dashboard_javascript` block to `dashboard` base template - Included `dashboard` JavaScript directly in `dashboard` templates - Removed `dashboard`-related entries in `main.js` - Remove references to `jquery.flot.min.js` from base template - This library is only used by the `dashboard` app - Removed `dashboard-*` CSS classes only used by `require.js` - Removed `require.js` structure in `dashboard` JavaScript files - Simplified structure of `utils.js`
8d20764
to
7ac24bc
Compare
This patch moves the
dashboard
app's JavaScript inclusion from the project'srequire.js
structure to the app itself.This was achieved with a new block in the base template called
javascript
. The rest of the site gets the default JavaScript includes, and thedashboard
app overrides the block to include the JavaScript specific to it. Thedashboard
app benefits by not knowing about JavaScript related to the rest of the site and vice versa.None of the actual
dashboard
JavaScript code was changed. It was only adapted to the new structure.javascript
block to base templatedashboard_javascript
block todashboard
base templatedashboard
JavaScript directly indashboard
templatesdashboard
-related entries inmain.js
jquery.flot.min.js
from base templatedashboard
appdashboard-*
CSS classes only used byrequire.js
require.js
structure indashboard
JavaScript filesutils.js