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 .click(), .focus() and .attr() jQuery functions with equivalent native JS #925

Merged
merged 7 commits into from
Nov 22, 2022

Conversation

dheerajdlalwani
Copy link
Contributor

@dheerajdlalwani dheerajdlalwani commented Nov 20, 2022

I have tested the changes in Edge Chromium, Chrome, Firefox and IE 11.

So far everything has been working. Opening PR. Let me know if any changes are required :)

Fixes #919 .

@dheerajdlalwani dheerajdlalwani marked this pull request as ready for review November 20, 2022 14:38
@Jaifroid Jaifroid self-requested a review November 20, 2022 15:37
Copy link
Member

@Jaifroid Jaifroid left a comment

Choose a reason for hiding this comment

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

Thank you very much for this PR. It's looking good! I haven't tested yet -- I've got some minor changes, mostly to do with quotation mark style, and a few cases where you could convert some other functions as well which don't look difficult to do. I'll test once you've made changes.

@Jaifroid Jaifroid added this to the v3.7 milestone Nov 20, 2022
@Jaifroid Jaifroid added the remove-jquery Issues or PRs involving removal of jQuery label Nov 20, 2022
@dheerajdlalwani
Copy link
Contributor Author

Thank you very much for this PR. It's looking good! I haven't tested yet -- I've got some minor changes, mostly to do with quotation mark style, and a few cases where you could convert some other functions as well which don't look difficult to do. I'll test once you've made changes.

Sure! I'll make the changes.

Copy link
Member

@Jaifroid Jaifroid left a comment

Choose a reason for hiding this comment

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

Nearly there -- just a few more small things!

Copy link
Member

@Jaifroid Jaifroid left a comment

Choose a reason for hiding this comment

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

This looks good, thanks (there is just one minor thing, removing a blank line). I'll do some testing later today, in particular on the effect of purging the iframe.

@dheerajdlalwani
Copy link
Contributor Author

This looks good, thanks (there is just one minor thing, removing a blank line). I'll do some testing later today, in particular on the effect of purging the iframe.

Great. I've removed the empty line. And sure, do test and let me know if there are any issues. I'll be most happy to resolve them :)

@dheerajdlalwani dheerajdlalwani force-pushed the remove-jquery-click-focus-attr branch from 209011a to edd185a Compare November 22, 2022 06:08
@dheerajdlalwani
Copy link
Contributor Author

I tested your suggestion, @Jaifroid and it works flawlessly! Thank you so much. Let me know if anything else is required.

Here, take a look!

image

@Jaifroid
Copy link
Member

@dheerajdlalwani Thank you very much, and also for testing thoroughly. It looks good now! I presume you don't see a button "Squash and merge" below the test results below? If you don't see it, or can't click it, let me know if you are ready to squash/merge, and I'll do it -- it will still appear on GitHub as your contribution/merge. NB it is important to squash down to a single commit, but GitHub can do this automatically.

@dheerajdlalwani
Copy link
Contributor Author

Thank you so much, @Jaifroid. No, I don't see the Squash and Merge button. I think you will have to do it. And you don't have to thank me! It was absolutely my pleasure. I'm learning so much from this! Infact, I am enjoying it!

@Jaifroid Jaifroid merged commit 7f8476d into kiwix:master Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup remove-jquery Issues or PRs involving removal of jQuery
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert jQuery .click(), .focus(), .attr() and other homologous functions to their native equivalents
2 participants