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

Update jQuery to v3.5.1. #1362

Merged
merged 3 commits into from
Jun 4, 2020
Merged

Update jQuery to v3.5.1. #1362

merged 3 commits into from
Jun 4, 2020

Conversation

XhmikosR
Copy link
Contributor

@XhmikosR XhmikosR commented May 23, 2020

Signed-off-by: XhmikosR xhmikosr@gmail.com

By submitting this pull request, I confirm the following: {please fill any appropriate checkboxes, e.g: [X]}

{Please ensure that your pull request is for the 'devel' branch!}

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)
  • I have Signed Off all commits. (git commit --signoff)

I tested it as much as I could and it seems to work OK. Things that could break are one of the jQuery plugins, so we should test them out as good as possible.

It's not urgent to update, but we should try to do it ASAP. There have been several bugxifes, security fixes and plenty of performance improvements.

Browser support should be what we currently support, and I doubt anyone sane would use IE < 9 😛: https://jquery.com/upgrade-guide/3.0/#browser-support

I'm going to go through each of the breaking changes and see if we have code that's using that.

https://jquery.com/upgrade-guide/3.0/

This would leave us 3rd-party code only.

TODO:

  • Test notify plugin
  • Test jquery confirm plugin
  • Test select plugin
  • Test toggle plugin
  • Address the .context() instances in our code
  • Address any newly added TODOs

Fixes #1014

@XhmikosR
Copy link
Contributor Author

I added a TODO in the OP.

From my tests, this has a clear positive effect on load. We should definitely do it after we test the TODO. 🙂

@XhmikosR
Copy link
Contributor Author

I'm going to split some of the patches that can land on develop later so that this branch is only left with the jQuery 3.x update.

@XhmikosR XhmikosR force-pushed the jquery-3 branch 3 times, most recently from 1db8d81 to 9d5ec02 Compare May 30, 2020 14:53
@XhmikosR
Copy link
Contributor Author

XhmikosR commented Jun 1, 2020

I'm gonna take a stab at fixing the TODO so that we can test this further, tops until tomorrow.

I'm not sure if we want to land this in v5.1.0, but the difference is pretty big IMO.

@XhmikosR XhmikosR force-pushed the jquery-3 branch 6 times, most recently from 568d517 to 0c28568 Compare June 3, 2020 14:42
@XhmikosR XhmikosR marked this pull request as ready for review June 3, 2020 14:59
@XhmikosR XhmikosR added this to the v5.1 milestone Jun 3, 2020
@XhmikosR XhmikosR requested review from PromoFaux and DL6ER June 3, 2020 15:07
@PromoFaux
Copy link
Member

Test select plugin

Not sure what I'm looking for to test here, any pointers welcome

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Jun 4, 2020

Should be OK, I meant bootstrap-select. But all of the plugins are up to date so they should be fine.

The only plugin that's old is jquery-confirm. So if that works too, we should be fine.

EDIT: for what is worth that's one of the reasons I have it in my TODO (#1352) to remove jquery-confirm and use Bootstrap's alerts. But if it still works, it's not something we need to do for 5.1.0.

XhmikosR added 3 commits June 4, 2020 14:49
Signed-off-by: XhmikosR <xhmikosr@gmail.com>
Signed-off-by: XhmikosR <xhmikosr@gmail.com>
Signed-off-by: XhmikosR <xhmikosr@gmail.com>
Copy link
Member

@PromoFaux PromoFaux left a comment

Choose a reason for hiding this comment

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

Tested on all pages, no unexpected errors in console log. Not sure if further testing is required...

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Jun 4, 2020

We should be OK hopefully. Unless I missed another breaking change, but hey, mistakes can happen and we can fix them if reported.

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Jun 4, 2020

I mean the 3.x BC are a lot, so I tried to check every one of them in our code: https://jquery.com/upgrade-guide/3.0/

The plugins are all updated, so that's something too.

@XhmikosR XhmikosR merged commit cd99d33 into devel Jun 4, 2020
@XhmikosR XhmikosR deleted the jquery-3 branch June 4, 2020 14:38
@yubiuser
Copy link
Member

Not sure if it fits here, but there were some changes concerning "hidden" in 3.5.1.

I see now this:
Bildschirmfoto zu 2020-06-12 10-40-43


@XhmikosR
Copy link
Contributor Author

I already mentioned this somewhere else, but I don't think it's related to this change. Either way, please open new issues and don't comment on old ones because it's hard to track stuff.

@XhmikosR
Copy link
Contributor Author

#1161 (comment)

@yubiuser
Copy link
Member

#1457

@PromoFaux PromoFaux mentioned this pull request Jul 5, 2020
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-5-1-released/35577/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants