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

Fix AdvancedFilterForm Media declaration #61

Merged
merged 1 commit into from
Dec 20, 2017

Conversation

metamatik
Copy link
Contributor

See #60 (comment) for context.

@asfaltboy, this is a quite empirical modification based on this Django documentation page.
It solves our deployment problem on Heroku, and I believe it shouldn't break anything anywhere else.

Feel free to integrate it upstream if you so wish :)

@coveralls
Copy link

coveralls commented Dec 20, 2017

Coverage Status

Coverage decreased (-0.007%) to 96.06% when pulling 997b319 on BureauxLocaux:fix-heroku-deployment into d7acb36 on modlinltd:develop.

@asfaltboy
Copy link
Member

@metamatik thank you for the contribution, very appreciated!

Interestingly enough, I see the docs are pretty much the same all the way back to Django 1.5 (assuming we still want to support that legacy Django version). This leaves me wondering whether there was ever any reason we used the static template tag this way...

The only thing I'm wondering about is the change of the js files order. I'm pretty sure jquery_adder.js needs to come before advanced-filters.js, or we'll have some cases where the wrong jQuery will be used, e.g when using Django-Grappelli (see #49 for details). Or, did you experience some sort of issue with the original order?

Besides the above, I don't see any reason not to merge this honestly.

@metamatik
Copy link
Contributor Author

You are much welcome, thank you for letting the community use your app :)

There is absolutely no valid reason for my reordering of the files other than my "pseudo-OCD", so I will cancel this change ^^

@metamatik metamatik force-pushed the fix-heroku-deployment branch from 997b319 to 78fc51b Compare December 20, 2017 16:14
@coveralls
Copy link

coveralls commented Dec 20, 2017

Coverage Status

Coverage decreased (-0.007%) to 96.06% when pulling 78fc51b on BureauxLocaux:fix-heroku-deployment into d7acb36 on modlinltd:develop.

@asfaltboy
Copy link
Member

Thank you again, that was quick! I'll merge this and keep it in develop for a while for you and @gone to be able to test it out. I'll slate this for a minor 1.0.7 release, which I hope to package this weekend.

@asfaltboy asfaltboy merged commit 699373b into modlinltd:develop Dec 20, 2017
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