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 usage of jquery while opening and closing bootstrap modals #968

Merged
merged 17 commits into from
Mar 26, 2023

Conversation

aritroCoder
Copy link
Contributor

Replaced use of jquery with native javascript calls while invoking and closing bootstrap modals. Although bootstrap 4.3 uses jquery under the hood, so complete removal is not possible until bootstrap is upgraded.
closes #921

@Jaifroid
Copy link
Member

Jaifroid commented Mar 4, 2023

@aritroCoder Thanks very much for this PR. I haven't reviewed it yet because there is an issue I identified straight away when I tested it in Firefox and Chromium locally. I tested by opening the app (with http-server on localhost:8080), clicking on the Home tab and pressing the random button. In both Firefox and Chromium (Edge) I get the Bootstrap modal shown in the screenshot below. However, clicking on OK does nothing. The modal remains open.

image

Please note that to test your code thoroughly, you will need to delete and refresh the Service Worker and Cache API caches, or else you may well find yourself testing the previous version, not your code changes. You can do this in-app by selecting "Bypass AppCache", opening Dev Tools, going to the Network tab and ensuring caching is turned off, then pressing Ctrl-Shift-R while DevTools is in focus. Then you should find any changes you make are in the app. For each new change, press Ctrl-Shift-R with DevTools focused.

When you ask for review, please include a description of how you have tested your fix, and in particular in which browsers. Many thanks!

@Jaifroid Jaifroid added user interface remove-jquery Issues or PRs involving removal of jQuery labels Mar 4, 2023
@Jaifroid Jaifroid added this to the v3.8 milestone Mar 4, 2023
@aritroCoder
Copy link
Contributor Author

@aritroCoder Thanks very much for this PR. I haven't reviewed it yet because there is an issue I identified straight away when I tested it in Firefox and Chromium locally. I tested by opening the app (with http-server on localhost:8080), clicking on the Home tab and pressing the random button. In both Firefox and Chromium (Edge) I get the Bootstrap modal shown in the screenshot below. However, clicking on OK does nothing. The modal remains open.

image

Please note that to test your code thoroughly, you will need to delete and refresh the Service Worker and Cache API caches, or else you may well find yourself testing the previous version, not your code changes. You can do this in-app by selecting "Bypass AppCache", opening Dev Tools, going to the Network tab and ensuring caching is turned off, then pressing Ctrl-Shift-R while DevTools is in focus. Then you should find any changes you make are in the app. For each new change, press Ctrl-Shift-R with DevTools focused.

When you ask for review, please include a description of how you have tested your fix, and in particular in which browsers. Many thanks!

Sure, I had tried this modal with the reset app button (in brave browser, which should give similiar result as chromium), it was working there, I will revert back shortly fixing this issue

@aritroCoder
Copy link
Contributor Author

aritroCoder commented Mar 5, 2023

@Jaifroid Sir, I have tested it with all the possible modals I could open, and it seems to work well in them (modals I checked are: the random button in home page, the reset cache modal, disable drag and drop, choosing non zim files from zim browser, swithching to jquery while selecting bypass appcache), in brave, google chrome, and edge browser.

@Jaifroid
Copy link
Member

Jaifroid commented Mar 7, 2023

@aritroCoder I'll take a look shortly. Thanks.

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.

@aritroCoder Thank you: it's mostly working! However, one piece of functionality is missing. You can test this by going to https://moz-extension.kiwix.org/current/www/index.html. In that version, you can click outside a modal to cancel it. In your version, clicking outside a modal does nothing. Test as follows:

  • Launch the app, but don't load a ZIM
  • Go to Home tab
  • Click Random button
  • When modal shows, try to cancel it by clicking OUTSIDE the modal. In the online version this works. In your version the modal stays showing and ignores the click.

@aritroCoder
Copy link
Contributor Author

@aritroCoder Thank you: it's mostly working! However, one piece of functionality is missing. You can test this by going to https://moz-extension.kiwix.org/current/www/index.html. In that version, you can click outside a modal to cancel it. In your version, clicking outside a modal does nothing. Test as follows:

  • Launch the app, but don't load a ZIM
  • Go to Home tab
  • Click Random button
  • When modal shows, try to cancel it by clicking OUTSIDE the modal. In the online version this works. In your version the modal stays showing and ignores the click.

Sure, I did it. I tested it with the same modals as in previous message

@aritroCoder
Copy link
Contributor Author

@Jaifroid please review the PR

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.

@aritroCoder Thank you for fixing the issue with clicking outside the dialogue box. That is working well now.

I've been testing, and I've run into another small issue: the original code handles pressing the Enter/Return key to select and pres Okay, and pressing the Esc key to Cancel. Your dialogue boxes don't handle these events. Nothing happens when I press either key.

Can you kindly correct this? You probably need to correct something in the code beginning line 124 of uiUtil.js in your PR. (And since you've already defined a constant modal for the dialogue box, you can probably re-use that in line 124 and below instead of doing document.getElementById('alertModal')....

www/js/lib/uiUtil.js Outdated Show resolved Hide resolved
www/js/lib/uiUtil.js Outdated Show resolved Hide resolved
@aritroCoder
Copy link
Contributor Author

Done, @Jaifroid

@Jaifroid Jaifroid mentioned this pull request Mar 15, 2023
29 tasks
www/js/lib/uiUtil.js Outdated Show resolved Hide resolved
www/js/lib/uiUtil.js Outdated Show resolved Hide resolved
www/js/lib/uiUtil.js Outdated Show resolved Hide resolved
@aritroCoder
Copy link
Contributor Author

@Jaifroid did you review it? I guess its good to be merged finally

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.

@aritroCoder Thank you, this is looking very good. There are just some style changes and suggested way of defining functions inside this function (using let xxx = function () { ... }; rather than function xxx() .... If you could implement the suggested changes, I'll do some testing of the final code. I think we're 99.9% there now!

www/js/lib/uiUtil.js Outdated Show resolved Hide resolved
www/js/lib/uiUtil.js Outdated Show resolved Hide resolved
www/js/lib/uiUtil.js Outdated Show resolved Hide resolved
www/js/lib/uiUtil.js Outdated Show resolved Hide resolved
www/js/lib/uiUtil.js Outdated Show resolved Hide resolved
www/js/lib/uiUtil.js Outdated Show resolved Hide resolved
www/js/lib/uiUtil.js Outdated Show resolved Hide resolved
www/js/lib/uiUtil.js Outdated Show resolved Hide resolved
@aritroCoder
Copy link
Contributor Author

@Jaifroid All of these should be fixed now

www/js/lib/uiUtil.js Outdated Show resolved Hide resolved
www/js/lib/uiUtil.js Outdated Show resolved Hide resolved
www/js/lib/uiUtil.js Outdated Show resolved Hide resolved
www/js/lib/uiUtil.js Outdated Show resolved Hide resolved
www/js/lib/uiUtil.js Outdated Show resolved Hide resolved
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.

@aritroCoder I've done a test with Edge and also (for backward compatibility) with IE11, and it all seems to be working great: keyboard, click events, clicking on the canvas. I need to test on an old Firefox OS that we support, but while I'm doing that could you kindly add the missing spaces as in the comments above? This is just for style consistency and readability (and it is what stylers like ESLint recommend). Once I've completed testing, and you've done those very minor edits, I'll squash and merge it for you. Many thanks.

@Jaifroid
Copy link
Member

PS I also checked that event listeners are not being added multiply: great job!

@aritroCoder
Copy link
Contributor Author

@aritroCoder I've done a test with Edge and also (for backward compatibility) with IE11, and it all seems to be working great: keyboard, click events, clicking on the canvas. I need to test on an old Firefox OS that we support, but while I'm doing that could you kindly add the missing spaces as in the comments above? This is just for style consistency and readability (and it is what stylers like ESLint recommend). Once I've completed testing, and you've done those very minor edits, I'll squash and merge it for you. Many thanks.

done

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.

@aritroCoder When testing on Firefox OS, I discovered that the let keyword isn't supported on that old OS (I had forgotten that). Apologies, as I misled you on this and had suggested using it. However, the solution is easy: change all let to var (lines 83 to 113). We can keep the two const that you have, as those are supported. Could you kindly make those changes? Other than that, all tested and working, so I can merge straight after.

@aritroCoder
Copy link
Contributor Author

@aritroCoder When testing on Firefox OS, I discovered that the let keyword isn't supported on that old OS (I had forgotten that). Apologies, as I misled you on this and had suggested using it. However, the solution is easy: change all let to var (lines 83 to 113). We can keep the two const that you have, as those are supported. Could you kindly make those changes? Other than that, all tested and working, so I can merge straight after.

Done

@Jaifroid
Copy link
Member

Excellent, all testing complete now! Will squash/merge. Many thanks for this PR which turned out to be more complex than I thought it would be.

@Jaifroid Jaifroid merged commit 0b2211c into kiwix:main Mar 26, 2023
@aritroCoder aritroCoder deleted the remove-jquery-where-possible branch March 26, 2023 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
remove-jquery Issues or PRs involving removal of jQuery user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove as much jQuery as possible from Bootstrap modal invocations
2 participants