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

Replace (some) jQuery code with native javascript #2013

Merged
merged 16 commits into from
Sep 24, 2023

Conversation

richardebeling
Copy link
Member

@richardebeling richardebeling commented Sep 4, 2023

These places are left, but they will have to be done in separate PRs:

  • Student voting page and sisyphus.js -- Sisyphus is based on jQuery, we'd probably have to replace it?
  • sortable_form_js.html / makeFormSortable / rowChanged / rowAdded -- I think this is enough for a separate PR.
  • bootstrap datetimepicker seems to be a jQuery-thing. Is there maybe a native alternative by now?
  • confirmation_text_modal.html and confirmation_modal.html -- @niklasmohrin is working on these and I don't know how to efficiently replace the unbind method call.

Todo:

  • templates/base.html still has jQuery usages in 172--178
  • evap_evaluation_edit_js.html
  • A few places had a fade-out-then-delete behavior. We want to get the fadeout back (search for slow in changes)
  • @niklasmohrin How to we want to make universal typescript helper functions available to templates? Currently, this affects assert and fadeOutThenRemove from utils.ts and CSRF_HEADERS from csrf_utils.ts. base.html includes utils to make these available globally.

@niklasmohrin
Copy link
Member

I don't think we can replicate unbind easily. If I understand correctly, it is only used to capture the data id variable, so a possible replacement would be to have a variable that the "show" handler updates. The unbind-bind then just becomes a single bind outside of the "show" handler

@richardebeling richardebeling marked this pull request as ready for review September 4, 2023 18:53
@niklasmohrin
Copy link
Member

If you want, you can also leave the modals unchanged and I take of them in the other PR. I havent exactly figured out how to deal with the "shared modals" business yet and wanted to think about that later, so if you have ideas / solutions I will gladly take them

@richardebeling
Copy link
Member Author

Yeah I decided that I will not bother with the actual base modal implementation in this PR, since that's what you are currently changing.

@niklasmohrin
Copy link
Member

The globalThis solution seems fine for now, we can think about it in another PR

evap/evaluation/templates/base.html Outdated Show resolved Hide resolved
evap/evaluation/templates/base.html Outdated Show resolved Hide resolved
evap/evaluation/templates/base.html Outdated Show resolved Hide resolved
evap/staff/templates/staff_semester_view.html Outdated Show resolved Hide resolved
evap/staff/templates/staff_semester_view.html Outdated Show resolved Hide resolved
evap/staff/templates/staff_semester_view.html Show resolved Hide resolved
evap/student/templates/student_vote.html Outdated Show resolved Hide resolved
evap/student/templates/student_vote.html Outdated Show resolved Hide resolved
Copy link
Member

@niklasmohrin niklasmohrin left a comment

Choose a reason for hiding this comment

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

Looks mostly good, some nits:

evap/evaluation/templates/base.html Outdated Show resolved Hide resolved
evap/evaluation/templates/evap_evaluation_edit_js.html Outdated Show resolved Hide resolved
evap/evaluation/templates/evap_evaluation_edit_js.html Outdated Show resolved Hide resolved
evap/student/templates/student_vote.html Outdated Show resolved Hide resolved
@richardebeling richardebeling merged commit 93cf241 into e-valuation:main Sep 24, 2023
11 of 12 checks passed
@richardebeling richardebeling deleted the remove-jquery branch September 24, 2023 11:34
richardebeling added a commit that referenced this pull request Oct 25, 2023
These were leftovers from #2013 where I forgot to remove them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants