A gift to javascript naysayers #897

juuz0 · 2023-02-12T23:15:05Z

Fixes #615

codecov · 2023-02-12T23:18:23Z

Codecov Report

❗ No coverage uploaded for pull request base (main@cb20317). Click here to learn what that means.
Patch coverage: 83.00% of modified lines in pull request are covered.

❗ Current head 55ba299 differs from pull request most recent head 25f589e. Consider uploading reports for the commit 25f589e to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #897   +/-   ##
=======================================
  Coverage        ?   72.31%           
=======================================
  Files           ?       56           
  Lines           ?     3887           
  Branches        ?     2185           
=======================================
  Hits            ?     2811           
  Misses          ?     1074           
  Partials        ?        2

Impacted Files	Coverage Δ
include/library.h	`100.00% <ø> (ø)`
src/library.cpp	`82.93% <0.00%> (ø)`
src/server/internalServer.h	`89.47% <ø> (ø)`
src/server/internalServer.cpp	`86.56% <55.00%> (ø)`
src/html_dumper.cpp	`96.15% <96.15%> (ø)`
include/library_dumper.h	`100.00% <100.00%> (ø)`
src/library_dumper.cpp	`100.00% <100.00%> (ø)`
src/opds_dumper.cpp	`98.92% <100.00%> (ø)`
src/server/i18n.h	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

juuz0 · 2023-02-19T11:01:58Z

@kelson42 I haven't written tests yet (still reading through how to write them, should be done soon along with some code polishing) but you can test the main functionality. Testing endpoint is TLD/nojs

Notes:
No tag filtering
For any type of filtering, you've to click on the "search" button. (less flexibility because no js)

kelson42 · 2023-02-19T14:23:40Z

@juuz0 Thank you very much for your PR. I have put @rgaudin as functional reviewer because he has requested the feature and has probably stronger opninion abuot how this shoudl be fixed. I have as well put @veloman-yunkan for the technical review (and functional as well :)

rgaudin · 2023-02-20T11:53:21Z

OK, I've tested it and it works fine ; congrats. Couple of comments though:

how is this going to be used? Right now, one has to go to /nojs manually to get this rendition. How will this be integrated for effective use? In a separate endpoint linked in a <noscript /> on the main page?
I understand the style was copied from the regular version but the magnification effect on a card's hovering only makes sense because it's clickable (and even then…) but in the nojs version it's not clickable and it thus just a distracting confusion.
where should those link point to? Currently it points to the viewer… which is JS dependent

@kelson42 what's the merge strategy for this?

veloman-yunkan

I skimmed through the changes very quickly and I think that it is yet premature commenting on the code. At this point I will provide high level input:

Shouldn't the output be paginated?
What about translation? Once Internationalization of kiwix-serve's frontend #846 is merged you can reuse the same strings for translating in the backend.

mgautierfr · 2023-02-21T14:18:49Z

where should those link point to? Currently it points to the viewer… which is JS dependent

/content/book_name/article may be a good endpoint. This is the content displayed in the iframe of the viewer.

mgautierfr · 2023-02-21T14:11:45Z

src/server/internalServer.cpp

+  auto filter = get_search_filter(request);
+  try {
+    if (request.get_argument("category") == "") {
+      filter.clearCategory();
+    }
+    if (request.get_argument("lang") == "") {
+      filter.clearLang();
+    }
+  } catch (...) {}


I'm surprised about that. In which context get_search_filter will set a filter on the category and we need to clear it ?
Shouldn't be better to fix get_search_filter if it is buggy ?

My reasoning for not touching get_search_filter was if the behaviour was intentional to display no books when lang/category == "" and if I change that logic, I would break some other functionality.
In kiwix-serve welcome page filters, "" means all categories while get_search_filter seems to consider "" as a valid category and rightly shows no book associated.

Would appreciate your input on how it should be done here? :)

I would say that get_search_filter behavior is correct. We should not pass a filter argument if we don't want to filter on this argument.

More over, we don't use kiwix-serve welcome page here as we are nojs here. When do we have a category=="" filter ?

I had assumed category=="" was for all categories filter
Is there a specific category for "all categories" then?

I'm not sure about what we are speaking about.
But if we are speaking about the request send to the http api, if we don't want to filter on the category, we must not pass a category=="" query string.

There needs to be a way for the server to know that the user requests "ALL CATEGORIES", I dont think a query param can be cleared without javascript on the client side hence I consider category="" as ALL categories & clear that on the server side.

There needs to be a way for the server to know that the user requests "ALL CATEGORIES"

The absence of filter on category seems to me a good way to specify all categories.

I dont think a query param can be cleared without javascript on the client side hence I consider category="" as ALL categories & clear that on the server side.

Why we need to clear it ?
The html code is generated server side, so we can generate the request we want, without a category parameter.

I dont think a query param can be cleared without javascript on the client side hence I consider category="" as ALL categories & clear that on the server side.

Why we need to clear it ? The html code is generated server side, so we can generate the request we want, without a category parameter.

On the server side we generate the HTML for the search form. The form is filled and submitted from the client side. Values are sent for all inputs of the form (unless it is a request generated via JS).

Ha yes, I've forget about the search form.
I would say that get_search_filter should be the only (and correct) way to create a filter from query string.
If we add a modification on the filter in specific api, it will introduce inconsistency in the API and we can be sure that we will mix things later.

while get_search_filter seems to consider "" as a valid category and rightly shows no book associated.

I finally think it is wrong. The only valid use case of searching for no category is to search for book which have no category associated. So it is more something about undefined category than category.

Maybe we could extend a bit the api to accept to specific values [all] and [empty] (or [none]) for no filtering (all) and filtering for book without category (empty/none).

The [] would prevent a potential conflict with a real category all or none.
What do you think about that ?

Extending the API would make life easier definitely & bring more consistency.
Would've been nice if all/none be reserved words for filters but I don't really mind the [all] or [empty/none] (none is better word imo).
Only consideration would be if this looks pleasant on the eye in URL: TLD/?lang=[all]&category=[none]

src/server/internalServer.cpp

juuz0 · 2023-02-22T15:08:13Z

how is this going to be used? Right now, one has to go to /nojs manually to get this rendition. How will this be integrated for effective use? In a separate endpoint linked in a on the main page?

Hiding nav, body, footer when javascript is not enabled & displaying a text "This page can only be accessed with Javascript enabled, please head over to <a href="{{root}}/nojs">/nojs endpoint</a> should be fine I think?

I understand the style was copied from the regular version but the magnification effect on a card's hovering only makes sense because it's clickable (and even then…) but in the nojs version it's not clickable and it thus just a distracting confusion.

It is clickable though? I had to reduce clickable area (currently it's only the description part) because HTML spec doesn't allow <a> nesting (another <a> is used for download button)

where should those link point to? Currently it points to the viewer… which is JS dependent

Will go by mgautierfr's suggestion, thanks!

Shouldn't the output be paginated?

normal welcome page (JS one) isn't paginated so why this? Performance reasons?

What about translation? Once Internationalization of kiwix-serve's frontend #846 is merged you can reuse the same strings for translating in the backend.

Noted, should be done soon - once I understand how to write tests which is taking some effort :)

rgaudin · 2023-02-22T15:15:20Z

Hiding nav, body, footer when javascript is not enabled & displaying a text "This page can only be accessed with Javascript enabled, please head over to <a href="{{root}}/nojs">/nojs endpoint</a> should be fine I think?

Fine by me. What do others think about?

It is clickable though? I had to reduce clickable area (currently it's only the description part) because HTML spec doesn't allow <a> nesting (another <a> is used for download button)

No it's not clickable. That's the point, it makes user think it is because it grows

Will go by mgautierfr's suggestion, thanks!

Way to go. This endpoint also handles the mainPath so that should be easy… although this URL should normally be retrieved from OPDS I suppose…

normal welcome page (JS one) isn't paginated so why this? Performance reasons?

It is. Just look at https://library.kiwix.org/ there's an automatic loading of new cards on scroll. I'm not in favor or paginating for a first version. It's unlikely this will be really useful in practice and adds some complexity. Let's wait for user requests for this.

juuz0 · 2023-02-23T19:58:37Z

how is this going to be used?......

Done as discussed.

I understand the style was copied from the regular....

Removed hover effect

where should those link point to? Currently it points to the viewer....

Done.

What about translation? .....

Still pending, I'm so scared to rebase this PR on top of main currently. Should be done soon.

Added two tests

juuz0 · 2023-02-23T20:00:21Z

dunno why CI is failing :(

juuz0 · 2023-02-25T08:19:13Z

What about translation? Once 846 is merged you can reuse the same strings for translating in the backend.

Done :)

juuz0 · 2023-02-25T08:20:11Z

@rgaudin ping because there's no button to re request a review! >:)

juuz0 · 2023-02-25T08:27:43Z

For translations, use userlang as url param.
Example
http://192.168.18.8:8080/nojs?lang=eng&userlang=test

rgaudin

Just tested 8c10936

Home points to /nojs as expected 👍
Links points to /content 👍
i18n translation works fine 👍
I still get the hover effect on the Tag: text is larger, bold and the pointer changes as well. ❌

veloman-yunkan

I hope that this was the last complete review of this PR. Please address the comments using fixup commits.

include/library_dumper.h

include/html_dumper.h

src/opds_dumper.cpp

veloman-yunkan · 2023-03-22T12:41:42Z

static/templates/no_js_library_page.html

@@ -51,7 +51,7 @@
        <div class="book__list">
        {{#books}}
        <div class="book__wrapper">
-            <a class="book__link" href="#" data-hover="Preview">
+            <a class="book__link" href="{{root}}/content/{{id}}" data-hover="Preview">


I guess that the data-hover attribute was copied from static/skin/index.js where it is supposedly used by isotope.js (however it doesn't seem to show up in the UI in any way). In plain HTML a different attribute must be used (of course, if we want such a hint/tooltip, whereupon I also have the following remarks:

Is "Preview" a good hint text?

It seems that it remained untranslated (this applies to the JS-ful version too).

Replaced this with title & aria-label.

Is "Preview" a good hint text?

Good enough to me to be honest. Preview book or Open book may also fit.

For the full JS version, I think its better to do in a new ticket? Or should I do that here too

For the full JS version, I think its better to do in a new ticket?

Yes, please

src/server/internalServer.cpp

static/skin/i18n/en.json

static/templates/no_js_download.html

test/server.cpp

veloman-yunkan

We are almost done. Please fix the last issue (discovered during final testing) in a rebased-and-fixed-up clean version of this PR ready to merge.

src/server/internalServer.cpp

veloman-yunkan · 2023-03-27T14:51:01Z

Please, also update static/skin/i18n/qqq.json (i.e. document the new messages there)

This change creates a new common class for dumping the library into various formats: LibraryDumper

HTMLDumper class will be used to dump library in HTML format. It inherits from LibraryDumper

Adds /nojs endpoint for fallback. Currently, it serves an HTML with book names in library

Tries to copy the same design of tiles as main page with javascript enabled

Adds span elements for tags

If the tiles are now clicked, they redirect to main page of book.

juuz0 · 2023-03-28T18:16:31Z

Please, also update static/skin/i18n/qqq.json (i.e. document the new messages there)

Done.

I also lost a commit while rebasing earlier, that change is re added in 3d3d695

(my question is: do we translate this text? what should be the preferred strategy because JS is disabled)

src/server/internalServer.cpp

The download-link links to /nojs/download/<bookname> for all 4 types of downloads.

Adds an html form to search books by the q= parameter

Shows "x results" label where x = number of books based on filters

Adds language and category filter in /nojs. Unlike the main page, the filtering is only done after user submits the form.

Uses the string from #846 for translations. A couple new translations are also added for <title> tag.

Shows a link to reset filter if there are no books.

Added 4 tests for /nojs endpoint Test 1: no_js_default - Without any filters Test 2: no_js_eng_lang - With lang=eng as filter Test 3: no_js_no_books - For 0 results case Test 4: no_js_download - To test download page

Added a <noscript> elements which hides everything on the welcome page if javascript is not enabled. It displays a text to tell the user to navigate to /nojs endpoint

juuz0 mentioned this pull request Feb 12, 2023

No JS fallback for kiwix-serve welcome page #615

Closed

juuz0 force-pushed the nojs branch 2 times, most recently from a462906 to 0838067 Compare February 19, 2023 10:26

juuz0 requested a review from kelson42 February 19, 2023 11:02

kelson42 requested review from rgaudin and veloman-yunkan February 19, 2023 14:22

kelson42 removed their request for review February 19, 2023 14:24

juuz0 force-pushed the nojs branch 2 times, most recently from e9687d6 to 9f6b559 Compare February 20, 2023 00:48

veloman-yunkan reviewed Feb 20, 2023

View reviewed changes

mgautierfr reviewed Feb 21, 2023

View reviewed changes

juuz0 force-pushed the nojs branch from 9f6b559 to 4c5862a Compare February 23, 2023 19:53

juuz0 force-pushed the nojs branch from 4c5862a to f189a6e Compare February 25, 2023 03:32

juuz0 marked this pull request as ready for review February 25, 2023 03:48

juuz0 force-pushed the nojs branch from f189a6e to 8c10936 Compare February 25, 2023 08:12

juuz0 requested review from veloman-yunkan and mgautierfr February 25, 2023 08:19

rgaudin requested changes Feb 27, 2023

View reviewed changes

juuz0 requested a review from veloman-yunkan March 22, 2023 12:33

veloman-yunkan requested changes Mar 22, 2023

View reviewed changes

juuz0 force-pushed the nojs branch from 1d660c1 to c1c6c01 Compare March 26, 2023 06:57

juuz0 requested a review from veloman-yunkan March 26, 2023 07:08

veloman-yunkan requested changes Mar 27, 2023

View reviewed changes

src/server/internalServer.cpp Outdated Show resolved Hide resolved

juuz0 added 6 commits March 28, 2023 20:25

Extract LibraryDumper from OPDSDumper

f187387

This change creates a new common class for dumping the library into various formats: LibraryDumper

Introduce HTMLDumper

d8656ec

HTMLDumper class will be used to dump library in HTML format. It inherits from LibraryDumper

Introduce /nojs endpoint

dca47d3

Adds /nojs endpoint for fallback. Currently, it serves an HTML with book names in library

Import book tiles

6071b98

Tries to copy the same design of tiles as main page with javascript enabled

Add tags in tiles for /nojs endpoint

c1d7cc3

Adds span elements for tags

Add links to content for tiles in /nojs

dbded6e

If the tiles are now clicked, they redirect to main page of book.

juuz0 force-pushed the nojs branch 2 times, most recently from ce93755 to 3d3d695 Compare March 28, 2023 17:58

juuz0 requested a review from veloman-yunkan March 28, 2023 18:15

veloman-yunkan reviewed Mar 29, 2023

View reviewed changes

src/server/internalServer.cpp Outdated Show resolved Hide resolved

juuz0 added 8 commits March 29, 2023 19:02

Add download-links to tiles in /nojs

0f7e11b

The download-link links to /nojs/download/<bookname> for all 4 types of downloads.

Add search form for /nojs endpoint

a48e2e6

Adds an html form to search books by the q= parameter

Add Results label

f843ea4

Shows "x results" label where x = number of books based on filters

language/category filtering in /nojs endpoint

37aadb8

Adds language and category filter in /nojs. Unlike the main page, the filtering is only done after user submits the form.

Translations for /nojs endpoint

cc35fe5

Uses the string from #846 for translations. A couple new translations are also added for <title> tag.

No results display in /nojs

951e15c

Shows a link to reset filter if there are no books.

Tests for /nojs

208f0f5

Added 4 tests for /nojs endpoint Test 1: no_js_default - Without any filters Test 2: no_js_eng_lang - With lang=eng as filter Test 3: no_js_no_books - For 0 results case Test 4: no_js_download - To test download page

noscript text on welcome page

25f589e

Added a <noscript> elements which hides everything on the welcome page if javascript is not enabled. It displays a text to tell the user to navigate to /nojs endpoint

juuz0 force-pushed the nojs branch from 3d3d695 to 25f589e Compare March 29, 2023 13:36

juuz0 requested a review from veloman-yunkan March 29, 2023 13:38

veloman-yunkan approved these changes Mar 29, 2023

View reviewed changes

kelson42 merged commit e13fed8 into main Mar 29, 2023

kelson42 deleted the nojs branch March 29, 2023 14:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

A gift to javascript naysayers #897

A gift to javascript naysayers #897

juuz0 commented Feb 12, 2023 •

edited

Loading

codecov bot commented Feb 12, 2023 •

edited

Loading

juuz0 commented Feb 19, 2023 •

edited

Loading

kelson42 commented Feb 19, 2023

rgaudin commented Feb 20, 2023

veloman-yunkan left a comment

mgautierfr commented Feb 21, 2023

mgautierfr Feb 21, 2023

juuz0 Feb 22, 2023 •

edited

Loading

mgautierfr Feb 27, 2023

juuz0 Feb 27, 2023

mgautierfr Mar 1, 2023

juuz0 Mar 2, 2023

mgautierfr Mar 7, 2023

veloman-yunkan Mar 7, 2023

mgautierfr Mar 8, 2023

juuz0 Mar 9, 2023

juuz0 commented Feb 22, 2023

rgaudin commented Feb 22, 2023

juuz0 commented Feb 23, 2023

juuz0 commented Feb 23, 2023

juuz0 commented Feb 25, 2023

juuz0 commented Feb 25, 2023

juuz0 commented Feb 25, 2023

rgaudin left a comment

veloman-yunkan left a comment

veloman-yunkan Mar 22, 2023

juuz0 Mar 26, 2023

veloman-yunkan Mar 27, 2023

veloman-yunkan left a comment

veloman-yunkan commented Mar 27, 2023

juuz0 commented Mar 28, 2023

A gift to javascript naysayers #897

A gift to javascript naysayers #897

Conversation

juuz0 commented Feb 12, 2023 • edited Loading

codecov bot commented Feb 12, 2023 • edited Loading

Codecov Report

juuz0 commented Feb 19, 2023 • edited Loading

kelson42 commented Feb 19, 2023

rgaudin commented Feb 20, 2023

veloman-yunkan left a comment

Choose a reason for hiding this comment

mgautierfr commented Feb 21, 2023

Choose a reason for hiding this comment

juuz0 Feb 22, 2023 • edited Loading

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 commented Feb 22, 2023

rgaudin commented Feb 22, 2023

juuz0 commented Feb 23, 2023

juuz0 commented Feb 23, 2023

juuz0 commented Feb 25, 2023

juuz0 commented Feb 25, 2023

juuz0 commented Feb 25, 2023

rgaudin left a comment

Choose a reason for hiding this comment

veloman-yunkan 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

veloman-yunkan left a comment

Choose a reason for hiding this comment

veloman-yunkan commented Mar 27, 2023

juuz0 commented Mar 28, 2023

juuz0 commented Feb 12, 2023 •

edited

Loading

codecov bot commented Feb 12, 2023 •

edited

Loading

juuz0 commented Feb 19, 2023 •

edited

Loading

juuz0 Feb 22, 2023 •

edited

Loading