Add tag filtering in kiwix serve #711

juuz0 · 2022-02-11T16:33:01Z

Fixes #582

codecov · 2022-02-11T16:34:32Z

Codecov Report

Merging #711 (45c4cd6) into master (b9e03d2) will not change coverage.
The diff coverage is n/a.

❗ Current head 45c4cd6 differs from pull request most recent head 43ab6df. Consider uploading reports for the commit 43ab6df to get more accurate results

@@           Coverage Diff           @@
##           master     #711   +/-   ##
=======================================
  Coverage   64.21%   64.21%           
=======================================
  Files          59       59           
  Lines        4102     4102           
  Branches     2227     2227           
=======================================
  Hits         2634     2634           
  Misses       1466     1466           
  Partials        2        2

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 b9e03d2...43ab6df. Read the comment docs.

kelson42

In the principle, this is what I meant. But we have to improve a few things:

"Search" button size is somehow changed now, should be the same size like before
The tag link should be a real link, and there should be clear visually that we can click on it
One link per tag, it seems it can only take two tags together (if we have two tags)

There is as well a few other visual details, but we can have a look to that later.

juuz0 · 2022-02-13T10:51:16Z

The tag link should be a real link, and there should be clear visually that we can click on it

Do you mean the real link as <a> tag?
Note that the whole book div is covered by another <a>, HTML specification doesn't allow nesting of anchor tags

kelson42 · 2022-02-13T10:59:00Z

@juuz0 Any chance we could try to reduce the surface of the tile link? Any way this is not a blocker if we can not have a real link.

juuz0 · 2022-02-13T11:39:50Z

@juuz0 Any chance we could try to reduce the surface of the tile link? Any way this is not a blocker if we can not have a real link.

Reducing surface could be neat, I'll try thanks

juuz0 · 2022-02-21T11:15:35Z

Rebased too

kelson42 · 2022-03-13T16:14:39Z

@juuz0 I consider to be an acceptable first version of this feature.
@veloman-yunkan Would you be able please to make a code review?

kelson42

LGTM

static/skin/index.js

veloman-yunkan · 2022-03-20T09:30:12Z

static/skin/index.js

+        const tagList = tags.split(';').filter(tag => {return !(tag.split(':')[0].startsWith('_'))})
+            .map((tag) => {return tag.charAt(0).toUpperCase() + tag.slice(1)});
+        const tagFilterLinks = tagList.map((tagValue) => {
+            tagValue = tagValue.toLowerCase().replace(/\s/g, '_');


Can tags contain whitespace?

Yes, the only forbidden char is ; https://wiki.openzim.org/wiki/Tags

Then the next question is - if whitespace is replaced with an underscore here, where is the effect of this transformation reversed?

in line 442,
using humanFriendlyTitle function

But the transformed tag value infiltrates the link (see the several lines below). I think that's incorrect.

Sorry, but I can't fathom how it infiltrates.

Let me explain it as inline comments in the generateTagLink() function:

function generateTagLink(tagValue) { tagValue = tagValue.toLowerCase().replace(/\s/g, '_'); // From now on tagValue has whitespace replaced with underscores let tempParams = new URLSearchParams(window.location.search); tempParams.set('tag', tagValue); // In tempParams the parameter 'tag' has a value where whitespace has been replaced with underscores const currentLink = `${window.location.href.split('?')[0]}?${tempParams.toString()}`; // currentLink was generated with a tag value where whitespace has been replaced with underscores // Therefore underscores (not originally present in the tag value) have infiltrated the link. QED. return `<a class='tag__link' href='${currentLink}'>${humanFriendlyTitle(tagValue)}</a>` }

That's intentional (replacing whitespaces with underscores in the link).
A tag like science in the bath can't be used like: tag=science in the bath. For that, I changed it to tag=science_in_the_bath which works

A tag like science in the bath can't be used like: tag=science in the bath. For that, I changed it to tag=science_in_the_bath which works

Is the treatment of underscores in the tag value a document feature? Which code does implement it? Is there a unit-test for it?

My bad, I didn't realise the tag value had underscores previously too so .replace(/\s/g, '_'); is useless.
The tag was always science_in_the_bath.
My reasoning for whitespaces is also incorrect, I will remove that whitespace-underscore replacement. Thanks!

static/skin/index.js

stale · 2022-03-30T05:53:53Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

veloman-yunkan

Please fix the last issue still remaining from the previous iteration (see #711 (comment))

veloman-yunkan

This time I played with the code instead of reviewing it. Here are a couple of observations:

When no books match the selection criteria "No result. Would you like to reset filter?" is shown. If filtering by tag was enabled (and the tag label was displayed), clicking the reset filter link doesn't remove the tag label (though filtering by tag gets disabled).
With tag filtering enabled, when I type text in the search box and click the search button, filtering by tag is preserved. However, if I type text in the search box and hit Enter, filtering by tag is removed.
Clicking the tag link resets any language and/or category filters.
History tracking doesn't work well (and that doesn't seem to be restricted to tags)

kelson42 · 2022-04-17T05:17:50Z

@juuz0 Any feedback?

veloman-yunkan · 2022-06-23T17:13:25Z

In fixup commit, I removed all the reset link logic, now it's only a simple link. In the onload handler, we are checking if the link with params is same as current, helps not to push the same thing twice. Seems to work well for all 3 you mentioned
1. open the kiwix-serve welcome/library page

2. click on a book or enter another (unrelated) URL (e.g. openzim.org) in the address bar

3. go back. Once you return to the kiwix-serve library, you cannot navigate forward in browsing history.

Great!

Now you have to make sure that the color of the filter fields is also properly updated during history navigation. For example, consider the following scenario:

Open the library without any filter applied. All fields are white.
Enter some garbage in the search field so that no book matches it. You end up with a gray search field and "No result. Would you like to reset filter" instead of a list of books.
Navigate back. The search field becomes empty but stays gray.
Navigate forward. The text in the search field is restored, but the field becomes white.

juuz0 · 2022-06-24T04:02:45Z

Now you have to make sure that the color of the filter fields is also properly updated......

Done ✔️

veloman-yunkan

Please also make the simplification recommended in the first part of #711 (comment) (as a separate clean-up commit) and rebase-fixup this PR for the last iteration.

veloman-yunkan

Please rewrite the history by moving all cleanup & refactoring changes to the beginning.

static/skin/index.js

veloman-yunkan

Almost at the finish line!

Yet, please address the new review comments in fixup commits.

static/skin/index.js

Adds a function to wrap logic to update select boxes on history change

Earlier we were using the full URL, now only query string is passed in pushState, much cleaner!

Previously, if the following steps were executed: 1. Click a book tile/visit an unrelated link from the address bar 2. Press back button Then forward history was discarded (forward button gets disabled). This happened because of the window.history.pushState on every window.onload event. This led to the same link being added in history and thus discarding the previous "forward-history" This change adds a condition to only push the current state if the queries are not same.

This removes the onclick handler around the reset-filter link which redirected to '/?lang=' Everything under the handler was already done on window.onload

Extracted the code from the un-named function in setTimeout for easier understanding.

This is done to retain the button design in more button designs (ex: tags)

Move hover behaviour as a different class - kiwixButtonHover Add cursor:pointer to kiwixButton

This change introduces filtering by tags. To filter, the user can click on the tag name and it will filter it. A label is added (clickable) to show the tag filter, it can be clicked to remove the filter

veloman-yunkan

Thank you for your patience!

@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 a review from kelson42 February 11, 2022 16:33

kelson42 requested changes Feb 12, 2022

View reviewed changes

juuz0 force-pushed the tagFilter branch from 0c0dad4 to 3b3505c Compare February 21, 2022 11:13

juuz0 requested a review from kelson42 February 21, 2022 11:14

juuz0 force-pushed the tagFilter branch from 3b3505c to 631b7d2 Compare February 21, 2022 11:15

kelson42 force-pushed the tagFilter branch from 631b7d2 to eedfb51 Compare March 13, 2022 16:00

kelson42 requested a review from veloman-yunkan March 13, 2022 16:14

kelson42 approved these changes Mar 13, 2022

View reviewed changes

juuz0 force-pushed the tagFilter branch from eedfb51 to 34c087b Compare March 13, 2022 17:35

veloman-yunkan requested changes Mar 13, 2022

View reviewed changes

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

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

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

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

kelson42 force-pushed the tagFilter branch from 34c087b to a74eb3e Compare March 18, 2022 06:05

juuz0 force-pushed the tagFilter branch from a74eb3e to 75e5fa1 Compare March 19, 2022 12:39

juuz0 requested a review from veloman-yunkan March 19, 2022 12:40

veloman-yunkan requested changes Mar 20, 2022

View reviewed changes

stale bot added stale and removed stale labels Mar 30, 2022

juuz0 force-pushed the tagFilter branch from 75e5fa1 to af7cb96 Compare April 2, 2022 07:37

juuz0 requested a review from veloman-yunkan April 2, 2022 07:37

veloman-yunkan requested changes Apr 3, 2022

View reviewed changes

juuz0 force-pushed the tagFilter branch from af7cb96 to ea28fd4 Compare April 12, 2022 05:18

juuz0 requested a review from veloman-yunkan April 12, 2022 05:18

veloman-yunkan requested changes Apr 12, 2022

View reviewed changes

juuz0 requested a review from veloman-yunkan June 23, 2022 16:33

veloman-yunkan reviewed Jun 24, 2022

View reviewed changes

juuz0 force-pushed the tagFilter branch from ecff9b7 to dcd2a49 Compare June 24, 2022 10:26

juuz0 requested a review from veloman-yunkan June 24, 2022 10:27

juuz0 force-pushed the tagFilter branch from dcd2a49 to 45c4cd6 Compare June 24, 2022 10:30

veloman-yunkan requested changes Jun 24, 2022

View reviewed changes

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

juuz0 force-pushed the tagFilter branch from 45c4cd6 to 5693eec Compare June 24, 2022 16:30

juuz0 requested a review from veloman-yunkan June 24, 2022 16:34

veloman-yunkan requested changes Jun 25, 2022

View reviewed changes

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

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

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

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

juuz0 force-pushed the tagFilter branch from 5693eec to 1696371 Compare June 25, 2022 10:49

juuz0 requested a review from veloman-yunkan June 25, 2022 10:50

veloman-yunkan reviewed Jun 25, 2022

View reviewed changes

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

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

juuz0 force-pushed the tagFilter branch from 1696371 to 509c234 Compare June 25, 2022 13:13

juuz0 requested a review from veloman-yunkan June 25, 2022 13:14

juuz0 added 8 commits June 25, 2022 18:10

Introduce updateVisibleParams()

947744c

Adds a function to wrap logic to update select boxes on history change

Use shortened URL in pushState

37b3943

Earlier we were using the full URL, now only query string is passed in pushState, much cleaner!

Drop onclick handler for reset-filter link

596b223

This removes the onclick handler around the reset-filter link which redirected to '/?lang=' Everything under the handler was already done on window.onload

Extract setNoResultsContent() from checkAndInjectEmptyMessage()

f034018

Extracted the code from the un-named function in setTimeout for easier understanding.

Change class name "searchButton" to "kiwixButton"

19a9c84

This is done to retain the button design in more button designs (ex: tags)

Refactoring kiwixButton

93f2686

Move hover behaviour as a different class - kiwixButtonHover Add cursor:pointer to kiwixButton

Add ability to filter by tags in kiwix serve

43ab6df

This change introduces filtering by tags. To filter, the user can click on the tag name and it will filter it. A label is added (clickable) to show the tag filter, it can be clicked to remove the filter

veloman-yunkan force-pushed the tagFilter branch from 509c234 to 43ab6df Compare June 25, 2022 14:10

veloman-yunkan approved these changes Jun 25, 2022

View reviewed changes

veloman-yunkan merged commit 12e0fb6 into master Jun 25, 2022

veloman-yunkan deleted the tagFilter branch June 25, 2022 14:22

mgautierfr mentioned this pull request Nov 30, 2022

New version 12.0.0 #850

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tag filtering in kiwix serve #711

Add tag filtering in kiwix serve #711

juuz0 commented Feb 11, 2022

codecov bot commented Feb 11, 2022 •

edited

Loading

kelson42 left a comment

juuz0 commented Feb 13, 2022

kelson42 commented Feb 13, 2022 •

edited

Loading

juuz0 commented Feb 13, 2022

juuz0 commented Feb 21, 2022

kelson42 commented Mar 13, 2022

kelson42 left a comment

veloman-yunkan Mar 20, 2022

mgautierfr Mar 21, 2022

veloman-yunkan Mar 21, 2022

juuz0 Mar 22, 2022

veloman-yunkan Mar 22, 2022

juuz0 Apr 2, 2022

veloman-yunkan Apr 3, 2022

juuz0 Apr 9, 2022

veloman-yunkan Apr 10, 2022

juuz0 Apr 12, 2022 •

edited

Loading

stale bot commented Mar 30, 2022

veloman-yunkan left a comment

veloman-yunkan left a comment

kelson42 commented Apr 17, 2022

veloman-yunkan commented Jun 23, 2022

juuz0 commented Jun 24, 2022

veloman-yunkan left a comment

veloman-yunkan left a comment

veloman-yunkan left a comment

veloman-yunkan left a comment

Add tag filtering in kiwix serve #711

Add tag filtering in kiwix serve #711

Conversation

juuz0 commented Feb 11, 2022

codecov bot commented Feb 11, 2022 • edited Loading

Codecov Report

kelson42 left a comment

Choose a reason for hiding this comment

juuz0 commented Feb 13, 2022

kelson42 commented Feb 13, 2022 • edited Loading

juuz0 commented Feb 13, 2022

juuz0 commented Feb 21, 2022

kelson42 commented Mar 13, 2022

kelson42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juuz0 Apr 12, 2022 • edited Loading

Choose a reason for hiding this comment

stale bot commented Mar 30, 2022

veloman-yunkan left a comment

Choose a reason for hiding this comment

veloman-yunkan left a comment

Choose a reason for hiding this comment

kelson42 commented Apr 17, 2022

veloman-yunkan commented Jun 23, 2022

juuz0 commented Jun 24, 2022

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 left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 11, 2022 •

edited

Loading

kelson42 commented Feb 13, 2022 •

edited

Loading

juuz0 Apr 12, 2022 •

edited

Loading