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

Removed javascript_libraries template tag #36119

Merged
merged 4 commits into from
Apr 1, 2025

Conversation

orangejenny
Copy link
Contributor

@orangejenny orangejenny commented Mar 31, 2025

Technical Summary

#36108 removed the last usage of this tag.

This does move a long chunk of code to hqwebapp/base.html, but it can be removed once all of app manager, including form builder, is on webpack.

Safety Assurance

Safety story

Removal of dead code.

Automated test coverage

no

QA Plan

no

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@orangejenny orangejenny added the product/invisible Change has no end-user visible impact label Mar 31, 2025
@orangejenny orangejenny requested a review from biyeun as a code owner March 31, 2025 18:43
@dimagimon dimagimon added Risk: High Change affects files that have been flagged as high risk. Risk: Medium Change affects files that have been flagged as medium risk. labels Mar 31, 2025
@@ -131,6 +131,104 @@

{% if not use_js_bundler %}
{% javascript_libraries use_bootstrap5=use_bootstrap5 underscore=True jquery_ui=request.use_jquery_ui ko=True hq=True analytics=True %}
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of adding a comment to this block to the effect of

{# Everything in this `if not use_js_bundler`-block below this line can be removed once all of app manager, including form builder, is on webpack. #}

It doesn't seem obvious, and seems like it could be easily forgotten.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will not forget 🙂 I've got a note to get rid of use_js_bundler altogether. But adding a comment sounds fine - 0969128

@orangejenny
Copy link
Contributor Author

I'm temporarily reverting #36073, so marking this "don't merge" until that's re-reverted.

@orangejenny orangejenny merged commit 32cb158 into master Apr 1, 2025
14 checks passed
@orangejenny orangejenny deleted the jls/remove-javascript-libraries-tag branch April 1, 2025 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/invisible Change has no end-user visible impact Risk: High Change affects files that have been flagged as high risk. Risk: Medium Change affects files that have been flagged as medium risk.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants