No jquery #796

juuz0 · 2022-07-13T20:26:21Z

Fixes #610
Fixes #719
Fixes #369

Supersedes #639

juuz0 · 2022-07-13T20:32:47Z

Didn't push in previous PR to not ruin it with my experiments :P
Rebased on master
I haven't rebased it on #716 yet, as @mgautierfr told to do so. Will see that.

As a change to previous taskbar suggestions, I went with a hyperlink in suggestions, this gives us the ability to copy/open link address in new tab just how wikipedia does. If this is not needed, will pull back.
Fixed suggestions, submit button

kelson42 · 2022-07-14T06:15:27Z

@juuz0 Please rebase and secure CI is OK. There a problem currently with the packages, but the rest should be green. In general, I'm not in favour of changing anything which is user visible, this ticket shiukd be purely technical.

test/server.cpp

codecov · 2022-07-14T15:32:31Z

Codecov Report

Merging #796 (2bc8e49) into master (98bcf8a) will not change coverage.
The diff coverage is n/a.

❗ Current head 2bc8e49 differs from pull request most recent head baf22c2. Consider uploading reports for the commit baf22c2 to get more accurate results

@@           Coverage Diff           @@
##           master     #796   +/-   ##
=======================================
  Coverage   70.65%   70.65%           
=======================================
  Files          53       53           
  Lines        3708     3708           
  Branches     2061     2061           
=======================================
  Hits         2620     2620           
  Misses       1086     1086           
  Partials        2        2

Impacted Files	Coverage Δ
src/opds_dumper.cpp	`99.13% <ø> (+0.86%)`	⬆️
src/server/response.cpp	`92.06% <0.00%> (-0.40%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98bcf8a...baf22c2. Read the comment docs.

kelson42 · 2022-07-19T03:56:36Z

@mgautierfr Can you please reivew this PR ?

test/server.cpp

static/templates/taskbar_part.html

static/skin/taskbar.css

static/skin/taskbar.js

kelson42 · 2022-07-20T19:34:32Z

@juuz0 Can you please rebase?

juuz0 · 2022-07-20T19:45:31Z

@kelson42 ✅

Question:
For the codefactor thing, it shows it can autofix, should we try that?

kelson42 · 2022-07-20T19:47:15Z

@juuz0 Honestly I prefer a manual fix.

juuz0 · 2022-07-20T20:02:39Z

@kelson42 done 👍

kelson42

From a user perspective, seems to work fine :)

kelson42 · 2022-07-21T07:27:00Z

@veloman-yunkan Considering that @mgautierfr is away for a while, could you please review this PR?

veloman-yunkan

Since I haven't reviewed this PR before, I would prefer to see its clean version (with fixup commits squashed). But I would also like to know the intended plan of dealing with the conflicting PR #716 - is this PR intended to be merged into master before #716 or it must be rebased on top of the latter?

static/resources_list.txt

static/skin/index.js

juuz0 · 2022-07-22T15:02:33Z

But I would also like to know the intended plan of dealing with the conflicting PR #716 - is this PR intended to be merged into master before #716 or it must be rebased on top of the latter?

I believe it is intended to be rebased on top of it later (#639 (comment))

kelson42 · 2022-07-22T15:37:32Z

This PR is old and simple in comparison to the iframe one, I would like to merge it ASAP. I see no reason to make the other way around.

veloman-yunkan

Selecting the suggestion using the keyboard (with arrow keys) and pressing ENTER runs a search for the text found in the searchbox instead of going to the suggested page.

Also I ran against an issue mostly unrelated to this PR but the previous version seemed to work fine under those circumstances. In my browser suggestions are not displayed nicely on the main page of the Ray Charles ZIM:

Also the styling of the taskbar slightly changes between the said page and other pages. Firefox reports the following warning in the console which probably explains the issue:

This page is in Quirks Mode. Page layout may be impacted. For Standards Mode use “”

Regarding the history of this PR - I would say that it is somewhat messy. The need for the new unit test in the context of this PR is not clear at all. Next, Jquery is removed first and the problems are then fixed. Such style of work can lead to problems if a bigger change is undertaken. A safer approach of dropping something is to first eliminate the dependency on it and only then removing it. So a better sequence would be:

Migrate the suggestions UI to autocomplete.js
Say thank you and good-bye to jquery.

static/resources_list.txt

static/skin/taskbar.js

veloman-yunkan

We've approached the final review. Please rebase-fixup and fix the history of the PR as follow:

I don't think we need the first two commits (the new unit test and the .gitignore) in this PR.
The commit titled "Update index.js to not use jquery anymore" should be taught good manners. Squeezing oneself between two logically related changes is impolite.
Having a separate change for fixing codefactor warnings is not justified.

test/server.cpp

static/skin/taskbar.js

static/resources_list.txt

veloman-yunkan

A few final touches

static/templates/head_taskbar.html

veloman-yunkan · 2022-07-31T08:06:09Z

static/skin/taskbar.js

+  let bookName = (window.location.pathname == `${root}/search`)
+    ? (new URLSearchParams(window.location.search)).get('content')
+    : window.location.pathname.split(`${root}/`)[1].split('/')[0];
+  bookName = encodeURIComponent(htmlDecode(bookName));


Why do you htmlDecode() the book name? Typically there will be no harm from it, but what if the ZIM file's name is &.zim? You only must decode data that is known to be encoded. Does any code in libkiwix HTML-encode the book name that goes into the URL?

Yeah, I shouldn't. I have also only url-encoded it in
const source = await fetch(${root}/suggest?content=${encodeURIComponent(bookName)}&term=${encodeURIComponent(query)});
now. If whole bookName was url encoded it caused problems with the zimfile&other.zim file.

static/skin/taskbar.js

Added autoComplete.css and .js files. Linked files in head_taskbar.html

This change only shows suggestions. Clicking them does nothing.

veloman-yunkan

In a PR with multiple commits and changes amounting to more than a couple dozen lines, please don't fix comments in place unless asked to do so. Fixup commits help the reviewer to quickly analyze all changes since the last review instead of trying to be more careful catching modifications in excess of what was requested.

static/skin/taskbar.js

veloman-yunkan

Great. Time to rebase, fixup and put the final cacheids!

The suggestions are now clickable hyperlinks.

Completes the porting of remaining jQuery code in taskbar.js - scroll function, blur and focus events and the cybook hack.

Now after porting index.js and taskbar.js to vanilla JS, it is time to remove files. Deleted static/skin/jquery-ui Updated customIndexPage template in README.md. Thank you for your service, jQuery :)

juuz0 · 2022-07-31T13:51:10Z

Done.
Now time for a doubt I had since forever,
what's the correct way to find cache IDs?
Currently, I compile, run the tests, let them fail. Go to testlog, find the correct cacheid -> replace. Is the only way?

veloman-yunkan · 2022-07-31T13:55:13Z

Now time for a doubt I had since forever, what's the correct way to find cache IDs? Currently, I compile, run the tests, let them fail. Go to testlog, find the correct cacheid -> replace. Is the only way?

That's how I do it too. The actual values of cacheids don't matter much. The important behaviour that is being checked is that whenever certain files change links to those resources get updated.

veloman-yunkan · 2022-07-31T13:58:19Z

From a user perspective, seems to work fine :)

@kelson42 Please validate the PR from user perspective once again before merging.

kelson42 · 2022-07-31T14:09:00Z

@veloman-yunkan I will, glad to see ir ready. Thx a lot for all the review work.

mgautierfr · 2022-08-02T14:54:40Z

Thanks to both of you to finish and cleanup this PR !

@mgautierfr

* [API Break] Remove wrapper around libzim (@mgautierfr #789) * Allow kiwix-serve to use custom resource files (@veloman-yunkan #779) * Properly handle searchProtocolPrefix when rendering search result (@veloman-yunkan #823) * Prevent search on multi language content (@veloman-yunkan #838) * Use new `zim::Archive::getMediaCount` from libzim (@mgautierfr #836) * Catalog: - Include tags in free text catalog search (@veloman-yunkan #802) - Illustration's url is based on book's uuid (@veloman-yunkan #804) - Cleanup of the opds-dumper (@veloman-yunkan #829) - Allow filtering of catalog content using multiple languages (@veloman-yunkan #841) - Make opds-dumper respect the namemapper (@mgautierfr #837) * Server: - Correctly handle `\` in suggestion json generation (@veloman-yunkan #843) - Better http caching (@veloman-yunkan #833) - Make `/suggest` endpoint thread-safe (@veloman-yunkan #834) - Better redirection of main page (@veloman-yunkan #827) - Remove jquery (@mgautierfr @juuz0 #796) - Better Viewer of zim content : . Introduce `/content` endpoints (@veloman-yunkan #806) . Switch to iframe based content viewer (@veloman-yunkan #716) - Optimised design of the welcome page: . Alignement (@juuz0 @kelson42 #786) . Exit download modal on pressing escape key (@Juzz0 #800) . Add favicon for different devices (@Juzz0 #805) . Fix auto hidding of the toolbar (@veloman-yunkan #821) . Allow user to filter books by tags in the front page (@juuz0 #711) * CI : - Trigger CI on pull_request (@kelson42 #791) - Drop Ubuntu Impish packaging (@legoktm #825) - Add Ubuntu Kinetic packaging (@legoktm #801) * Testing: - Test ICULanguageInfo (@veloman-yunkan #795) - Introduce fake `test` language to test i18n (@veloman-yunkan #848) * Fix documentation (@kelson42 #816) * Udpate translation (#787 #839 #847)

juuz0 requested review from mgautierfr and kelson42 July 13, 2022 20:33

mgautierfr reviewed Jul 14, 2022

View reviewed changes

test/server.cpp Outdated Show resolved Hide resolved

juuz0 force-pushed the noJquery branch from 1bc7a41 to ff8e531 Compare July 14, 2022 15:08

juuz0 requested a review from mgautierfr July 14, 2022 15:12

mgautierfr reviewed Jul 14, 2022

View reviewed changes

test/server.cpp Outdated Show resolved Hide resolved

juuz0 force-pushed the noJquery branch from ff8e531 to 7fc467a Compare July 14, 2022 15:27

juuz0 requested a review from mgautierfr July 14, 2022 15:27

mgautierfr requested changes Jul 19, 2022

View reviewed changes

test/server.cpp Outdated Show resolved Hide resolved

static/templates/taskbar_part.html Outdated Show resolved Hide resolved

static/skin/taskbar.css Outdated Show resolved Hide resolved

static/skin/taskbar.js Outdated Show resolved Hide resolved

juuz0 requested a review from mgautierfr July 19, 2022 15:57

juuz0 force-pushed the noJquery branch from a0358df to d347c15 Compare July 20, 2022 19:42

juuz0 force-pushed the noJquery branch from 6818db9 to 496597e Compare July 20, 2022 20:01

kelson42 approved these changes Jul 21, 2022

View reviewed changes

kelson42 requested a review from veloman-yunkan July 21, 2022 07:26

veloman-yunkan reviewed Jul 22, 2022

View reviewed changes

static/resources_list.txt Outdated Show resolved Hide resolved

static/skin/index.js Show resolved Hide resolved

juuz0 force-pushed the noJquery branch from 496597e to bd093fb Compare July 23, 2022 05:08

juuz0 requested a review from veloman-yunkan July 23, 2022 05:31

veloman-yunkan requested changes Jul 23, 2022

View reviewed changes

static/resources_list.txt Outdated Show resolved Hide resolved

static/skin/taskbar.js Outdated Show resolved Hide resolved

static/skin/taskbar.js Outdated Show resolved Hide resolved

juuz0 force-pushed the noJquery branch from bd093fb to 20d9515 Compare July 24, 2022 19:29

veloman-yunkan requested changes Jul 30, 2022

View reviewed changes

Update index.js to not use jquery anymore.

4f3ec81

juuz0 force-pushed the noJquery branch from 2bc8e49 to e4e64b3 Compare July 30, 2022 21:47

juuz0 requested a review from veloman-yunkan July 30, 2022 21:52

veloman-yunkan requested changes Jul 31, 2022

View reviewed changes

juuz0 and others added 2 commits July 31, 2022 16:16

Welcome, autoComplete.js

1e247d7

Added autoComplete.css and .js files. Linked files in head_taskbar.html

Show suggestions using autoComplete.js

f0f473b

This change only shows suggestions. Clicking them does nothing.

juuz0 force-pushed the noJquery branch from e4e64b3 to 99e9a64 Compare July 31, 2022 12:19

juuz0 requested a review from veloman-yunkan July 31, 2022 12:22

veloman-yunkan requested changes Jul 31, 2022

View reviewed changes

static/skin/taskbar.js Outdated Show resolved Hide resolved

juuz0 requested a review from veloman-yunkan July 31, 2022 13:34

veloman-yunkan reviewed Jul 31, 2022

View reviewed changes

juuz0 added 3 commits July 31, 2022 19:11

Turn suggestions into hyperlinks

a0db199

The suggestions are now clickable hyperlinks.

Implement taskbar scroll actions in vanilla JS

f8a5301

Completes the porting of remaining jQuery code in taskbar.js - scroll function, blur and focus events and the cybook hack.

So long, jQuery

baf22c2

Now after porting index.js and taskbar.js to vanilla JS, it is time to remove files. Deleted static/skin/jquery-ui Updated customIndexPage template in README.md. Thank you for your service, jQuery :)

juuz0 force-pushed the noJquery branch from 6d6a997 to baf22c2 Compare July 31, 2022 13:48

juuz0 requested a review from veloman-yunkan July 31, 2022 13:49

veloman-yunkan approved these changes Jul 31, 2022

View reviewed changes

kelson42 merged commit 4cd52b0 into master Aug 1, 2022

kelson42 deleted the noJquery branch August 1, 2022 13:15

veloman-yunkan mentioned this pull request Aug 4, 2022

Iframe-based content viewer #716

Merged

veloman-yunkan mentioned this pull request Sep 14, 2022

autoComplete.min.js is already minified in the code repository #819

Closed

mgautierfr mentioned this pull request Nov 30, 2022

New version 12.0.0 #850

Merged

veloman-yunkan mentioned this pull request Dec 7, 2022

Add unminified version of autoComplete.js and attribution #852

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No jquery #796

No jquery #796

juuz0 commented Jul 13, 2022 •

edited by kelson42

Loading

juuz0 commented Jul 13, 2022

kelson42 commented Jul 14, 2022 •

edited

Loading

codecov bot commented Jul 14, 2022 •

edited

Loading

kelson42 commented Jul 19, 2022

kelson42 commented Jul 20, 2022

juuz0 commented Jul 20, 2022

kelson42 commented Jul 20, 2022

juuz0 commented Jul 20, 2022

kelson42 left a comment

kelson42 commented Jul 21, 2022

veloman-yunkan left a comment

juuz0 commented Jul 22, 2022

kelson42 commented Jul 22, 2022 •

edited

Loading

veloman-yunkan left a comment

veloman-yunkan left a comment

veloman-yunkan left a comment

veloman-yunkan Jul 31, 2022

juuz0 Jul 31, 2022

veloman-yunkan left a comment

veloman-yunkan left a comment

juuz0 commented Jul 31, 2022

veloman-yunkan commented Jul 31, 2022

veloman-yunkan commented Jul 31, 2022

kelson42 commented Jul 31, 2022

mgautierfr commented Aug 2, 2022

No jquery #796

No jquery #796

Conversation

juuz0 commented Jul 13, 2022 • edited by kelson42 Loading

juuz0 commented Jul 13, 2022

kelson42 commented Jul 14, 2022 • edited Loading

codecov bot commented Jul 14, 2022 • edited Loading

Codecov Report

kelson42 commented Jul 19, 2022

kelson42 commented Jul 20, 2022

juuz0 commented Jul 20, 2022

kelson42 commented Jul 20, 2022

juuz0 commented Jul 20, 2022

kelson42 left a comment

Choose a reason for hiding this comment

kelson42 commented Jul 21, 2022

veloman-yunkan left a comment

Choose a reason for hiding this comment

juuz0 commented Jul 22, 2022

kelson42 commented Jul 22, 2022 • edited Loading

veloman-yunkan left a comment

Choose a reason for hiding this comment

veloman-yunkan left a comment

Choose a reason for hiding this comment

veloman-yunkan left a comment

Choose a reason for hiding this comment

veloman-yunkan Jul 31, 2022

Choose a reason for hiding this comment

juuz0 Jul 31, 2022

Choose a reason for hiding this comment

veloman-yunkan left a comment

Choose a reason for hiding this comment

veloman-yunkan left a comment

Choose a reason for hiding this comment

juuz0 commented Jul 31, 2022

veloman-yunkan commented Jul 31, 2022

veloman-yunkan commented Jul 31, 2022

kelson42 commented Jul 31, 2022

mgautierfr commented Aug 2, 2022

juuz0 commented Jul 13, 2022 •

edited by kelson42

Loading

kelson42 commented Jul 14, 2022 •

edited

Loading

codecov bot commented Jul 14, 2022 •

edited

Loading

kelson42 commented Jul 22, 2022 •

edited

Loading