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

Load jQuery once and only once, in page foot #399

Merged
merged 7 commits into from
Feb 1, 2018
Merged

Conversation

stefanor
Copy link
Member

I found a small herd of hairy yaks while looking at a JS error on our talk submission page:

Uncaught TypeError: element.markItUp is not a function

The form was rendering inline script and CSS tags, that depended on JS that we were only loading at the end of the page.

Fixing some of this is fairly simple:

  1. Pages with forms get slightly more complex, and are instructed to render CSS in the HEAD, and JS in the page foot.
  2. Widgets are instructed to not load their own jQuery, because we provide one.

However, markitup renders inline JS that requires jQuery and markitup's JS library that defeats this strategy (zsiciarz/django-markitup#25) so I implemented a template override to work around it. I'll propose the same changes upstream.

For performance, this is a best practice.

We already do this for our own JS, which includes jQuery. Howover, some
form JS needs jQuery, so it needs to be loaded after jQuery.
See: zsiciarz/django-markitup#25

django-markitup renders inline JS in each widget that configures the
widget. This JS requires jQuery and markitup.js to already be loaded,
which isn't the case if we're loading JS in the foot of the page.

So, override the template that generates that inline JS, with something
that generates a hidden tag containing only config. Then come back at
the end of the JS loading, and configure the widgets.

I'll do a similar PR upstream and hope that we can pull this out, later.
@@ -14,3 +17,6 @@ <h1>{% trans 'Edit User:' %}</h1>
{% endif %}
{% crispy form %}
{% endblock %}
{% block extra_foot %}
{{ form.media.js }}
{% endblock %}
Copy link
Member

Choose a reason for hiding this comment

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

We're repeating this boilerplate in a lot of places now - is it worthwhile to look at consolidating things into a base_form template ?

<div class="django-markitup-config" style="display: none"
data-element="#{{ textarea_id }}"
data-preview-url="{{ preview_url }}"
data-auto-preview="{{ AUTO_PREVIEW|yesno:"1,0" }}"></div>
Copy link
Member

Choose a reason for hiding this comment

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

How does this change interact with Markitup's Admin widget?

Won't we need to also ensure the javascript snippet is included on the admin pages so this works there too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. I wanted to test that too :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Resolved that by hooking into wafer/compare/templates/admin/wafer.compare/change_form.html. But that's a temporary hack if upstream will accept a similar change.

Copy link
Member

Choose a reason for hiding this comment

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

Playing with this branch, it looks like we aren't doing the right thing for the markup fields used in the sponsors admin page, but otherwise this looks good.

Copy link
Member Author

Choose a reason for hiding this comment

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

Overrode that admin template too.

@stefanor
Copy link
Member Author

Upstreamed the markitup hacks as zsiciarz/django-markitup#26 (although upstream seems pretty dead these days)

Copy link
Member

@drnlm drnlm left a comment

Choose a reason for hiding this comment

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

Looks OK to me now.

@stefanor stefanor merged commit d0021a6 into master Feb 1, 2018
@stefanor stefanor deleted the double-jquery branch February 1, 2018 08:44
@stefanor
Copy link
Member Author

stefanor commented Feb 1, 2018

If this is ever fixed upstream, I propose merging https://github.com/CTPUG/wafer/compare/roll-back-markitup-hacks

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.

None yet

2 participants