Bootstrap version updated to remove jQuery dependency

[AritraLeo]

These are my initial changes so far and I'd am open to update any of the other files in case it's needed.

Before opening this PR I have -

performed e2e testing for chrome and edge

also build the project locally

Please have a look !
issue - #1233

[Jaifroid]

Thank you! I'm just running automated tests.

[AritraLeo]

Thank you! I'm just running automated tests.

Sure also please let me know if any changes are required, this is my first PR to your project I am just being cautious!

[Jaifroid]

Looking good so far! I see this error in console.log, so you need to remove JQuery from the Service Worker's precache array.

But, more seriously, when the screen is narrow and the menus have collapsed, the toggle no longer seems to be working. You probably have to adjust the commands in app.js (possibly uiUtil.js) to whatever new syntax is needed for Bootstrap 5.

[Jaifroid]

NB to see the precache error, you probably have to reset the app, so it erases the SW and then reloads it.

[Jaifroid]

We probably need to tweak the styles of the buttons, because hovering over them now makes them greyed out rather than darker as previously, which looks odd.

[AritraLeo]

We probably need to tweak the styles of the buttons, because hovering over them now makes them greyed out rather than darker as previously, which looks odd.

Ohk I assume this is due to newer version of bootstrap have to tweak the styling and all.
But can you please provide me with little more info about -
"NB to see the precache error, you probably have to reset the app, so it erases the SW and then reloads it."

[Jaifroid]

It's explained in CONTRIBUTING -- the app caches its own code (in the Service Worker). To see if the error is solved, you'll need to use the Reset button (under Expert Settings) to erase the old Service Worker and the Cache, and then watch in console.log as it repopulates the Cache according to the code in ´service-worker.js`. There are other ways to do this, like forcing a reload of the Service Worker on refresh (an option under Application in Dev Tools). Note that after resetting the app, you should turn on the option to Bypass AppCache again (Expert Settings).

[AritraLeo]

@Jaifroid
Thank you so much for such a nice a response really appreciate it!
I'll surely update the PR with the necessary changes and let you know.

[AritraLeo]

@Jaifroid I found the issue for the toggle actually there was nothing to change in app/uUtil.js but bootstrap uses it's own attrs now with bs. Like data-bs-toggle. And removed the jQuery from precache running tests now (also setup my dev env to update cache on reload - I skipped that part on CONTRIBUTING file my bad). Also checked the button hover thing.

If there's anything else please let me know !

Is this ideal ?

[Jaifroid]

Is this ideal ?

Did you test it in dark mode (and you should probably turn off the bypass appCache option when testing the buttons at least in dark mode, as it tends to paint things red)? My initial reaction is that a somewhat more subtle colour change would be preferable to the button turning black! Maybe a somewhat darker grey instead?

[Jaifroid]

I found the issue for the toggle actually there was nothing to change in app/uUtil.js but bootstrap uses it's own attrs now with bs. Like data-bs-toggle. And removed the jQuery from precache running tests now (also setup my dev env to update cache on reload - I skipped that part on CONTRIBUTING file my bad). Also checked the button hover thing.

If there's anything else please let me know !

Great! Could you push your code so I can test? You also might want to update the branch (see "Update branch" button below), and don't forget to pull locally so that the update gets added to your PR before pushing more code.

[AritraLeo]

I found the issue for the toggle actually there was nothing to change in app/uUtil.js but bootstrap uses it's own attrs now with bs. Like data-bs-toggle. And removed the jQuery from precache running tests now (also setup my dev env to update cache on reload - I skipped that part on CONTRIBUTING file my bad). Also checked the button hover thing.
If there's anything else please let me know !

Great! Could you push your code so I can test? You also might want to update the branch (see "Update branch" button below), and don't forget to pull locally so that the update gets added to your PR before pushing more code.

Sure I'll push my code will give the button issue a check in dark mode and push my changes, and ya I mostly update my branch and pull locally but hey thanks for the tip appreciate it!

[AritraLeo]

I found the issue for the toggle actually there was nothing to change in app/uUtil.js but bootstrap uses it's own attrs now with bs. Like data-bs-toggle. And removed the jQuery from precache running tests now (also setup my dev env to update cache on reload - I skipped that part on CONTRIBUTING file my bad). Also checked the button hover thing.
If there's anything else please let me know !

Great! Could you push your code so I can test? You also might want to update the branch (see "Update branch" button below), and don't forget to pull locally so that the update gets added to your PR before pushing more code.

Actually I have used Bootstrap 5's btn-dark because others were not apt. I didn't tweak the styling but ya sure I can.
Just let me know the hex code you want for hover I'll try to incorporate it.

[AritraLeo]

@Jaifroid
In dark mode -

Without hover

On hover.

I have pushed the changes for now I have used the secondary btn class for the front 3 buttons only if you like it, I'll change it everywhere.

[AritraLeo]

@Jaifroid please share your thoughts!

[Jaifroid]

Thanks for the changes, and apologies for the delay (I was away travelling). I like the hover colour of the button you changed, but there's something wrong with the sizing of it compared to the other buttons, and it appears greyed out when not selected, which I don't think is a good idea:

I also noticed some differences between the Bootstrap 5 version and the original version, which I've circled in the side-by-side comparison below. Could you please try to remediate these? The spacing of the word "Kiwix" looks particularly bad, as it's lost its margin.

[AritraLeo]

@Jaifroid no issues.
I'll surely take a look asap and try to fix the issues you've pointed out for me. Will notify you when I push these changes.
Thank You!

[AritraLeo]

Hey @Jaifroid I've made the necessary changes so far but I am afraid that complete migration to Bootstrap 5 may require even more changes. In case you find something else please let me know I'll try to resolve any other issue asap.
Thank You!

[Jaifroid]

Hi @AritraLeo, a lot of things are fixed here, but I noticed a new problem: the search field has now become very short! See screenshot:

You should look into fixing this with Bootstrap methods ideally rather than just forcing a new style of the search bar in app.css, though of course you should check if an existing style rule exists in app.css that may be conflicting with Bootstrap 5.

[AritraLeo]

@Jaifroid I was concerned about these minor bugs which will pop up anyway. So I think I'll work on it for now but I'll just add it to my doc as well .

[Jaifroid]

OK, no problem. I suggest you update the branch (you should see "Update branch" button below).

[AritraLeo]

OK, no problem. I suggest you update the branch (you should see "Update branch" button below).

Sure I'll (saw 86 issues it was 87 yesterday knew something got resolved) Thanks for the tip

[Jaifroid]

There's a thing I forgot to mention that needs doing: remove jquery from the patch_gitignore.sh script under /scripts. Basically, just remove any of the lines that have jquery anywhere in the line. To understand what this does, look at ADDING_DEPENDENCIES_NODE_MODULES.md.

[AritraLeo]

There's a thing I forgot to mention that needs doing: remove jquery from the patch_gitignore.sh script under /scripts. Basically, just remove any of the lines that have jquery anywhere in the line. To understand what this does, look at ADDING_DEPENDENCIES_NODE_MODULES.md.

Sure! I'll get it done.

[AritraLeo]

Hi @AritraLeo, a lot of things are fixed here, but I noticed a new problem: the search field has now become very short! See screenshot:

You should look into fixing this with Bootstrap methods ideally rather than just forcing a new style of the search bar in app.css, though of course you should check if an existing style rule exists in app.css that may be conflicting with Bootstrap 5.

Hey @Jaifroid

I am unable to find this issue in my setup
Is there any specific reason for that ?

[Jaifroid]

@AritraLeo I think you need to fully refresh your Service Worker to ensure your browser is showing the latest code. You can test by opening an "In Private" or "Incognito" window and loading the app in there. I'm seeing the issue in Edge and in Firefox.

[AritraLeo]

@AritraLeo I think you need to fully refresh your Service Worker to ensure your browser is showing the latest code. You can test by opening an "In Private" or "Incognito" window and loading the app in there. I'm seeing the issue in Edge and in Firefox.

Sure I'll try it in Firefox with Hard Reload let's see.

[AritraLeo]

@Jaifroid
I am still unable to find this issue (using Edge)
Can you please provide me with a way to refresh the cache maybe in some other way? But I don't think there's any issue with cache because I am running the server again after executing a Hard reload.

[Jaifroid]

For starters, you don't have the "Bypass AppCache" setting turned on. This is necessary. Under the Network tab, did you make sure "Disable cache" is turned on as advised in CONTRIBUTING? With DevTools open, use Ctrl-Shift-R to hard-refresh. This always works with Chrome / Edge if Bypass AppCache is on and caching disabled. Firefox is trickier, I'm afraid, and you might have to delete the Service Worker manually (in Dev Tools -> Application tab) and also the two Cache API Caches.

However, as I mentioned earlier, a sure-fire way to see the issue without doing any of those things is simply to put your browser into InPrivate / Incognito / "new private window" mode. Then load the localhost address. Did you try that? In Firefox, you can tell if you've enabled this (see screenshot):

[AritraLeo]

For starters, you don't have the "Bypass AppCache" setting turned on. This is necessary. Under the Network tab, did you make sure "Disable cache" is turned on as advised in CONTRIBUTING? With DevTools open, use Ctrl-Shift-R to hard-refresh. This always works with Chrome / Edge if Bypass AppCache is on and caching disabled. Firefox is trickier, I'm afraid, and you might have to delete the Service Worker manually (in Dev Tools -> Application tab) and also the two Cache API Caches.

However, as I mentioned earlier, a sure-fire way to see the issue without doing any of those things is simply to put your browser into InPrivate / Incognito / "new private window" mode. Then load the localhost address. Did you try that? In Firefox, you can tell if you've enabled this (see screenshot):

Yeah I tried with Hard Reload and all the things you've mentioned I'll just try all of this once more and delete the worker file this time I think it'll work

[Jaifroid]

If you tried all the things mentioned (though Bypass AppCache was not turned on in your screenshot), and you tried the browser's Private mode, then I can only conclude we are not testing the same code. Perhaps your local code has not been pushed fully to this branch. Ensure your local branch is not dirty (no uncommitted changes), and ensure you have pulled/pushed the branch before testing again. I'm consistently seeing the error on both Firefox and Chrome. I also suggest you update the branch again and push/pull, and test in Chrome/Edge as well as Firefox.

[AritraLeo]

If you tried all the things mentioned (though Bypass AppCache was not turned on in your screenshot), and you tried the browser's Private mode, then I can only conclude we are not testing the same code. Perhaps your local code has not been pushed fully to this branch. Ensure your local branch is not dirty (no uncommitted changes), and ensure you have pulled/pushed the branch before testing again. I'm consistently seeing the error on both Firefox and Chrome. I also suggest you update the branch again and push/pull, and test in Chrome/Edge as well as Firefox.

Yeah I know I am going wrong somewhere. Just trying to find it.
Anyways I'll get back to you with the issue resolved this time !

[Jaifroid]

@AritraLeo As mentioned on Slack, I've discovered that the error is an artefact of my not having re-updated the software with npm install after switching branches to review someone else's work. Hence, my system was loading an older version of Bootstrap when I switched back to your branch.

Having corrected that, the short search bar issue has gone. I apologize for that oversight.

[AritraLeo]

@AritraLeo As mentioned on Slack, I've discovered that the error is an artefact of my not having re-updated the software with npm install after switching branches to review someone else's work. Hence, my system was loading an older version of Bootstrap when I switched back to your branch.

Having corrected that, the short search bar issue has gone. I apologize for that oversight.

Yeah! I see no issues with that.

[Jaifroid]

@AritraLeo Sorry for the hiatus. Getting back to this PR, here are some very minor code-format things, and I'll list the other non-code issues I'm seeing in the next comment.

[AritraLeo]

I'll get them done @Jaifroid.

[Jaifroid]

Here are some other issues I noticed:

1. There are some oddities on the home screen (see screenshot). Most important here is that the icons on the bottom bar are not correctly spaced as they are in the currently released version;
2. The spacing generally is too compacted in Configuration. You can compare side-by-side the currently released browser extension and this PR, and you'll see what I mean. We have to have adequate spacing to cater for touchscreen support;
3. You still need to remove jQuery from: Check-OfflineFilesList.ps1 and patch_gitignore.sh;
4. There's a dangling comma in service-worker.js line 153;
5. Please update the branch to the latest.

[AritraLeo]

@Jaifroid Hey so sorry for having no updates on PR (you know why). I have got the none code issues resolved have a look I'll start with the other issue mentioned and get the tasks done asap. Thanks!

[Jaifroid]

Apart from resolving the conflicts, take a look at the reasons for the Codefactor failure: https://github.com/kiwix/kiwix-js/pull/1235/checks?check_run_id=25892549156 . They're usually just formatting fixes.

[Jaifroid]

It's looking a lot better now, and is nearly there! There is just the issue of overly compact spacing in the version in your PR. This is minor, but we have to bear in mind that the app is used also on touchscreen devices. I've drawn some lines for you to compare the spacing at the connected points.

This should be easy to fix, either by wrapping some elements in paragraphs, or by adding appropriate spacing for the affected elements in app.css. Then we'll be pretty much there, I think!

[AritraLeo]

@Jaifroid I'll just check with Bootstrap v5 docs again if there's some class name that's deprecated or not. Else make changes to the app.css file. I'll update the branch as per required and get back to you.