-
-
Notifications
You must be signed in to change notification settings - Fork 56
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
Add tag filtering in kiwix serve #711
Conversation
Codecov Report
@@ 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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Do you mean the real link as |
@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 |
Rebased too |
@juuz0 I consider to be an acceptable first version of this feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
static/skin/index.js
Outdated
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, '_'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can tags contain whitespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the only forbidden char is ;
https://wiki.openzim.org/wiki/Tags
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then the next question is - if whitespace is replaced with an underscore here, where is the effect of this transformation reversed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in line 442,
using humanFriendlyTitle
function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the transformed tag value infiltrates the link (see the several lines below). I think that's incorrect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but I can't fathom how it infiltrates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>`
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A tag like
science in the bath
can't be used like:tag=science in the bath
. For that, I changed it totag=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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the last issue still remaining from the previous iteration (see #711 (comment))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
@juuz0 Any feedback? |
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:
|
Done ✔️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rewrite the history by moving all cleanup & refactoring changes to the beginning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost at the finish line!
Yet, please address the new review comments in fixup commits.
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
509c234
to
43ab6df
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your patience!
* [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)
Fixes #582