-
-
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 filters to kiwix-serve welcome page #534
Conversation
@MananJethwani Great, yes please make the same category/list like on Kiwix Desktop. |
@kelson42 I have added 2 json files for filter lists. |
@MananJethwani This branch should be rebased on |
52ff120
to
9602f1b
Compare
@kelson42 done |
@MananJethwani Will you please summarize in the description of the PR your approach to filtering? |
|
@MananJethwani Thank you for the elaborate description. In fact it contains too much detail. The purpose of a summary is to omit the details and outline the high-level ideas. Then, with the big picture in mind, it will be easier to analyze the implementation during the review. |
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.
@MananJethwani Few things to change:
- This branch is not rebased properly (it is on master) and contains old commits handling the welcome page on OPDS (please secure that you check this all the time). As a consequence I'm not able to test it easily (without having a library with URLs).
- The categories should have the first letter uppercase (in label). This is not "wikipedia", this is "Wikipedia"
- "Categories" should be replaced with "All categories" and "Languages" with "All languages"
- I would like that the language box preselect the top language given by the browser. Please secure languages codes like "pt-PT" fallback properly to "pt".
- Secure that first loading of the page just loads all the content in the language of the user
- Can we hav that the "Powered by Kiwix" is always at the bottom of the screen and that if no result are display with display "no result" in the middle of the page?
BTW @veloman-yunkan We really need kiwix/kiwix-tools#357 implemented as well. Like you can see here, the language filter is in English, which is almost useless for anybody not speaking English.
7994e3d
to
02748c9
Compare
@kelson42 for this feature
if we don't have any content in this particular language we can show a URL/Button to go back to show complete data would that be a nice feature? |
Yes. A message like "No result. Would you like to reset filter?". |
a518847
to
19d5609
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.
- Codefactor does not pass!
- By changing many times filters which each does not return any result I get this:
- "Powered by Kiwix" is still not at the bottom of the screen (and "no result" not in the middle of the screen either.
- Language list is still not (was it at some point?) in native language (everything is in english)
- IMO this special language
ltz
is not optimal. "All language" filter should belang=
. You should be able to make the difference with lang been unspecified.
c057e27
to
1d63d81
Compare
@MananJethwani Thank you, the problems reported recently have been fixed and it starts to be really fancy. Two small improvements I would like to see implemented (one of then I have talked earlier):
|
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.
The logic of this PR is too complex for me to provide valuable feedback at this time. I will have to try the code in action and look at it again. This iteration has yielded only some insignificant comments.
@veloman-yunkan if you want we can have a proper discussion on slack regarding the logic and how I can work on improving it. |
57dffb9
to
f4a83ed
Compare
@veloman-yunkan if I go with your described way then none of the effects applied by isotope will work the way they should, the main effect applied by isotope on filtering is the transition one which makes element move from one position to another but, for that to occur the reference for that element should be same meaning that element can't be discarded from the screen. If I discarded it and added it again, it will have a zoom out and zoom in effect in place of the transition effect. |
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.
- I don't see any fadeout at the bottom on Firefox
- If I choose "All languages" and then reload then I come back with the "French" filter. It should not.
- The spinner works, but the look and feel is not really nice. It generates a kind of falsh effect which is annoying. I would prefer to (1) not flash (2) display the spinner in place of the result new tiles, as long as they are not displayed.
@MananJethwani Thanks. That makes sense. It would be helpful if you provide a real summary (rather than a detailed description of the implementation) of this enhancement including all updates since then. The summary must be put in the description of the PR and kept up to date if significant changes are made. In this case, please structure the summary as follows:
Ideally, the need for such explanation could be reduced if the functionality could be demonstrated with reviewable automated tests. This however is too much to ask from this PR, since that requires a new kind of testing infrastructure. At least you can provide a list of test scenarios for manual testing with supporting data. |
ba1cd8e
to
7d785f4
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.
Works fine now for me. Good job,
@veloman-yunkan Here are the features added by this PR:
Hope this helps. |
@veloman-yunkan I have updated the PR description as you asked |
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.
At least one bug (repeated twice) has to be fixed. Other than that, looks like the next review will be the last one.
bfff648
to
01ea3a6
Compare
@MananJethwani @Popolechien We will have to handle properly font size in next PR. |
@kelson42 this resolves #531, #517 and #519,
@veloman-yunkan this PR focuses on adding filter library and effects related to subset loading and filtering, this PR also creates a base for other filters which are yet to be implemented. the filter options are derived from a static list for now.
All of these features are listed by kelson here
As for the implementation part, this PR uses
isotope.js
for filtering as suggested by kelson,Isotope
is used for the effects it adds.the functionality was achieved using the following implementation -
resetAndFilter()
is called which resets all basic configuration and callsloadAndDisplayBooks()
.loadAndDisplayBooks()
fetches books based on the filters applied using OPDS, then it calls isotope filter and uses it to hide the elements that are not present in books retrieved for the current filters(the books are not yet deleted but just hidden by isotope), then the hidden books are deleted and if the books that were fetched had more books then currently on-screen remaining books are inserted as well.