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]

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.

[Jaifroid]

Feel free to re-open if you want to complete this.

[AritraLeo]

Feel free to re-open if you want to complete this.

I am extremely sorry for taking so long. I want to get back to this PR tomorrow morning and get the changes done (the merge to be precise). Apologies again. (Reason in your dm)

[Jaifroid]

@AritraLeo Thank you very much for continuing work on the PR. For some reason, tests are looking pretty broken, and not just with the usual timeout issue. See https://github.com/kiwix/kiwix-js/actions/runs/10201928029/job/28225674553?pr=1235 and https://github.com/kiwix/kiwix-js/actions/runs/10201928029/job/28225674843?pr=1235. You can debug locally with npm test, or open tests/index.html (with Vite running) in the browser you want to test. See https://github.com/kiwix/kiwix-js/blob/main/TESTS.md for more info.

[Jaifroid]

Sorry, that command was for Unit tests. But you probably need to debug e2e tests as well. See https://github.com/kiwix/kiwix-js/blob/main/TESTS.md#end-to-end-tests. To test e2r on Chrome, it's npm run test-e2e-chrome.

[AritraLeo]

Sorry, that command was for Unit tests. But you probably need to debug e2e tests as well. See https://github.com/kiwix/kiwix-js/blob/main/TESTS.md#end-to-end-tests. To test e2r on Chrome, it's npm run test-e2e-chrome.

Ohk so I'll have to check why the e2e tests are failing right? Only this one?

[Jaifroid]

Ohk so I'll have to check why the e2e tests are failing right? Only this one?

Well you could run local tests in Firefox if you prefer. npm run test-e2e-firefox. The Firefox test didn't run because the Chrome test failed. I'll bet it encounters the same issue, whatever it is.

[Jaifroid]

This PR was passing tests prior to 8f91d7a, so looking at the code you changed there might give you a clue as to why tests are failing consistently since.

[AritraLeo]

This PR was passing tests prior to 8f91d7a, so looking at the code you changed there might give you a clue as to why tests are failing consistently since.

Yeah sure I'll look into it I was not in good health recently and also got caught up in my job. Spare me a bit more time. Apologies for taking so long again!

[Jaifroid]

Oh, sorry to hear that, no problem of course!

[AritraLeo]

Oh, sorry to hear that, no problem of course!

Please check dm in slack