Skip to content
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

Merged
merged 7 commits into from
Jun 7, 2021
Merged

Add filters to kiwix-serve welcome page #534

merged 7 commits into from
Jun 7, 2021

Conversation

MananJethwani
Copy link
Contributor

@MananJethwani MananJethwani commented May 20, 2021

@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 -

  • when users clicks on filter resetAndFilter() is called which resets all basic configuration and calls loadAndDisplayBooks().
  • 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.
  • the books need to preserve the order they came in from OPDS response so I have used the isotopes sorting feature and added an index to each book and used it for sorting(handled automatically by isotope).
  • Why haven't I just removed all current results from the screen and added all from the OPDS response? If I do so, the isotope's effect won't work properly as isotope has a transition effect, and for that to be performed, the element should have the same reference and shouldn't be re-rendered.
  • And as for why I first hide and then delete the element? If I just remove it while filtering is going on then there is a problem with the layout caused by isotope(most probably because some elements are then skipped during iteration).

@kelson42
Copy link
Collaborator

@MananJethwani Great, yes please make the same category/list like on Kiwix Desktop.

@MananJethwani
Copy link
Contributor Author

@kelson42 I have added 2 json files for filter lists.

@kelson42
Copy link
Collaborator

@MananJethwani This branch should be rebased on adding_dynamic_and_subset_loading

@MananJethwani
Copy link
Contributor Author

@kelson42 done

@veloman-yunkan
Copy link
Collaborator

@MananJethwani Will you please summarize in the description of the PR your approach to filtering?

@MananJethwani
Copy link
Contributor Author

@veloman-yunkan

  • I added a new parameter to server-side code to restrict it from inserting a taskbar if the server has to respond with the home page.
  • I added code in index.html to implement a basic navbar with filters and a search option.
  • as for filtering, I am using isotope.js as suggested by kelson for its animations and effects.
  • when a user selects a filter, filterBooks() is called and it updates the params in URL which are then used to create the OPDS URL later on by queryUrlBuilder() it also updates the basicConfig["start"] to 0 so that only first subset is loaded, then it calls for the loadAndDisplayBooks() which has a new param sort(I will explain about it later on).
  • loadAndDisplayBooks() will first call loadBooks() which will fetch and return books data and it will iterate over its entries and will pair up bookMap with an id of that particular book as the key and index at which it came in the response as the value(index will be used by sort).
  • At this point, I need to explain how the isotope filter actually works, isotope doesn't delete the data from the DOM it just changes its display property and moves it to the bottom of the screen so when I try to scroll down, the subset loading won't be called as document.body.offsetHeight will be greater(this part is used by the condition in subset loading). to solve this I removed all the hidden elements and left others there. P.S. - I can't remove all elements as if I do that then transitio effect applied by isotope won't work as for an object to give the transition we need it must be present at the screen with the same reference in isotope.
  • using the above method there was one more problem, consider If I searched for as a query w then searched wiki then again w as I deleted the top results for w earlier and am just showing wiki so when I bring back all data for w they would be appended and order would be changed, to solve this I used the sorting part.
  • for the sorting part we need to apply it only once when the first subset is loaded and only when a filter is applied, so sorting is set to true In only that case for loadDisplayBooks() and in that case data bookMap() is cleared and loadBooks() will add an index for each id for the first subset only which will be then added as a data-idx attribute and used by the isotope weight function to sort the data.
  • after that when iso.arrange() is called I have specified a special filter function that will check if an element is present in bookMap then it needs to be kept and otherwise it needs to be first hidden and also added to a booksToDelete set which will delete it later on(we can't delete it there only as else it will interfere with isotope filter's working).
  • I also had to keep a track of which books were present and do not need to append them again and for that purpose, I created a booksToFilter set that kept track of those books.

@veloman-yunkan
Copy link
Collaborator

@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.

Copy link
Collaborator

@kelson42 kelson42 left a 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.

@MananJethwani MananJethwani force-pushed the filter_library branch 2 times, most recently from 7994e3d to 02748c9 Compare May 27, 2021 13:03
@MananJethwani
Copy link
Contributor Author

MananJethwani commented May 27, 2021

@kelson42 for this feature

Secure that first loading of the page just loads all the content in the language of the user
and
Can we have that the "Powered by Kiwix" is always at the bottom of the screen and that if no result is display with display "no result" in the middle of the page?

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?

@kelson42
Copy link
Collaborator

Yes. A message like "No result. Would you like to reset filter?".

Copy link
Collaborator

@kelson42 kelson42 left a 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:
    image
  • "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 be lang=. You should be able to make the difference with lang been unspecified.

@MananJethwani MananJethwani force-pushed the filter_library branch 2 times, most recently from c057e27 to 1d63d81 Compare May 30, 2021 15:50
@kelson42 kelson42 changed the title Filter library Add filters to kiwix-serve welcome page May 31, 2021
@kelson42
Copy link
Collaborator

@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):

  • Add a spider (in the middle of the screen) as long as the results are not rendered
  • We should have a fadeout at the bottom if there is more results to be displayed. Otherwise, like now, the "Powered by Kiwix".

Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a 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.

static/skin/index.js Outdated Show resolved Hide resolved
static/skin/index.js Outdated Show resolved Hide resolved
static/skin/index.js Show resolved Hide resolved
@MananJethwani
Copy link
Contributor Author

@veloman-yunkan if you want we can have a proper discussion on slack regarding the logic and how I can work on improving it.

@MananJethwani
Copy link
Contributor Author

@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.
Isotope is not a CSS animation library it's a filter library that can filter data on basis of some defined attributes.

Copy link
Collaborator

@kelson42 kelson42 left a 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.

@veloman-yunkan
Copy link
Collaborator

veloman-yunkan commented Jun 2, 2021

@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.
Isotope is not a CSS animation library it's a filter library that can filter data on basis of some defined attributes.

@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:

  1. Targeted functionality. When writing this part, try to abstract yourself from your implementation. Only describe how you intended things to work. You may include/mention (with a special note) things that were not implemented in this PR but which you had in mind.
  2. High-level outline of the implementation achieving the desired functionality. Describe difficulties that you encountered, all significant options that you considered for solving those issues and why you chose the solutions found in the current implementation.

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.

Copy link
Collaborator

@kelson42 kelson42 left a 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,

@kelson42
Copy link
Collaborator

kelson42 commented Jun 5, 2021

@veloman-yunkan Here are the features added by this PR:

  • Remove taskbar in welcome page, search/suggestion was not working anyway
  • New filters by language, category, free text
  • Language filter is configured per default like the browser language
  • New fadeout at the bottom of results if still new results can be loaded via scrolling
  • Make “powered by kiwix” absolute at bottom of the page
  • “No result” in middle of the page with filter result link
  • Spinner during new results loading time
  • Perfect aync beetween URL arguments and real filter

Hope this helps.

@MananJethwani
Copy link
Contributor Author

@veloman-yunkan I have updated the PR description as you asked

Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a 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.

static/skin/index.js Show resolved Hide resolved
static/skin/index.js Show resolved Hide resolved
static/skin/index.js Show resolved Hide resolved
static/skin/index.js Show resolved Hide resolved
@Popolechien
Copy link
Member

Mobile display is not practical - everything is too small (particularly category selectors which as shown below are smaller than the phone clock's font for instance) and not readable unless one manually zooms in.
Screenshot_20210607-124630

@kelson42
Copy link
Collaborator

kelson42 commented Jun 7, 2021

@MananJethwani @Popolechien We will have to handle properly font size in next PR.

@kelson42 kelson42 merged commit 2ef4888 into master Jun 7, 2021
@kelson42 kelson42 deleted the filter_library branch June 7, 2021 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants