Add widget support by juuz0 · Pull Request #797 · kiwix/libkiwix

juuz0 · 2022-07-16T16:57:37Z

Widget endpoint: /widget
Fixes #585

codecov · 2022-07-16T16:58:55Z

Codecov Report

Merging #797 (4cd52b0) into master (8e6d893) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 4cd52b0 differs from pull request most recent head f42d773. Consider uploading reports for the commit f42d773 to get more accurate results

@@           Coverage Diff           @@
##           master     #797   +/-   ##
=======================================
  Coverage   70.62%   70.63%           
=======================================
  Files          53       53           
  Lines        3707     3708    +1     
  Branches     2060     2061    +1     
=======================================
+ Hits         2618     2619    +1     
  Misses       1087     1087           
  Partials        2        2

Impacted Files	Coverage Δ
src/server/internalServer_catalog_v2.cpp	`92.94% <0.00%> (+0.08%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us.

juuz0 · 2022-07-16T17:02:28Z

Will fix the code coverage 👍
Obviously not completed but I think we can review it from a user perspective with its current form and if this the way to move forward

mgautierfr · 2022-07-19T14:14:27Z

I have simply done a quick review as this is still in draft.

We should not have a endpoint in the server to test widget feature. Adding it extend the server with a endpoint served at runtime for the user which is totally useless.
Widget feature should not be a runtime feature of js. Server must serve a different content dedicated to widgets when requested on /widget endpoint. (And as we return different content, we don't need a widgettest endpoint to test the behavior when the content is put in a Iframe)

kelson42 · 2022-07-20T17:44:27Z

@juuz0 I don't see any changes folowing @mgautierfr! Any problem? How should I test the feature? Where is the documentation?

juuz0 · 2022-07-28T21:39:30Z

@kelson42 https://libkiwix--797.org.readthedocs.build/en/797/widget.html

kelson42 · 2022-08-01T13:19:17Z

@juuz0 Can you please rebase (and resolve the conflicts) now that we have merged the "no jquery" PR?

juuz0 · 2022-08-01T14:33:55Z

@kelson42 Done.

juuz0 · 2022-08-06T14:47:07Z

@kelson42 Any updates here? :)

Extracts function updateBookCount() from the unnamed function in resize event.

Adds an endpoint /widget to provide kiwix serve widget.

Gives a name to the IIFE wrapping code in index.js - kiwixServe and exposes updateBookCount through it

kelson42

Thank you @juuz0 For this first version. It start to be like I expected. Here a few question/remarks:

The CSS example does not work for me (i see nothing in red). Are you sure it is OK?
We should have a way to filter normaly (via URL) like on library.kiwix.org, it seems to not be the case
The ability to show one or many ZIM files based on their name (M/Name) should work as well on welcome page (and should probably be done via a dedicated PR).
http://localhost:8181/widget?book=gutenberg_fa_all_2022-01 does not deliver only one book with the M/Name value = gutenberg_fa_all_2022-01

juuz0 · 2022-08-14T16:34:35Z

The CSS example does not work for me (i see nothing in red). Are you sure it is OK?

I've checked again, it works on my side with the example in the docs. What browser do you use? I tried on Vivaldi (Chromium based) and Firefox. Exact code of html file?

We should have a way to filter normaly (via URL) like on library.kiwix.org, it seems to not be the case

These work for me again!

http://localhost:8181/widget?book=gutenberg_fa_all_2022-01 does not deliver only one book with the M/Name value = gutenberg_fa_all_2022-01

Yes I confirm this mistake, happened during a recent rebase.

The ability to show one or many ZIM files based on their name (M/Name) should work as well on welcome page (and should probably be done via a dedicated PR).

Looking through the C++ code, I have found there's indeed a way to filer by book name already. Though with two problems:

It doesn't work (http://192.168.18.8:8080/?name=gutenberg_fa_all_2022-04 should give the book but it doesn't, I have found the fix and will be fixing it in the dedicated PR)
It isn't made to accept multiple values which we want here, doing that too.

kelson42 · 2022-08-14T17:26:31Z

The CSS example does not work for me (i see nothing in red). Are you sure it is OK?

I've checked again, it works on my side with the example in the docs. What browser do you use? I tried on Vivaldi (Chromium based) and Firefox. Exact code of html file?

I test with Firefox, the code comes from the your documentation:

$ cat widget.html 
<!DOCTYPE html>
<html lang="en">
<head>
<title>Widget Test</title>
</head>
<body>
  <iframe src="http://localhost:8181/widget" width=1000 height=1000></iframe>

  <script>
window.onload = function() {
var receiver = document.getElementById('receiver').contentWindow;
function sendMessage() {
    let msg = {
    css: `
    .book__header {
        color:red;
    }`,
    js: `
    function widgetTest() {
    console.log("Testing widget");
    }
    widgetTest();
    `
    }
    receiver.postMessage(msg, 'http://192.168.18.8:8080/widget');
}
sendMessage();
}
</script>

</body>
</html>

and the result looks like

I see nothing which is different than without custom CSS. Considering that there is nothing which has the DOM id="receiver", I suspect the documentation to be simply wrong or incomplete.

We should have a way to filter normaly (via URL) like on library.kiwix.org, it seems to not be the case

These work for me again!

Indeed, seems to work. Sorry I can not reproduce the problem.

http://localhost:8181/widget?book=gutenberg_fa_all_2022-01 does not deliver only one book with the M/Name value = gutenberg_fa_all_2022-01

Yes I confirm this mistake, happened during a recent rebase.

The ability to show one or many ZIM files based on their name (M/Name) should work as well on welcome page (and should probably be done via a dedicated PR).

Looking through the C++ code, I have found there's indeed a way to filer by book name already. Though with two problems:
1. It doesn't work (`http://192.168.18.8:8080/?name=gutenberg_fa_all_2022-04` should give the book but it doesn't, I have found the fix and will be fixing it in the dedicated PR)

2. It isn't made to accept multiple values which we want here, doing that too.

All of this seems to be the same problem and actually is 100% independent of the widget system IMO. Can you please open a ticket dedicated to this (and also work on the solution).

An other detail, "Powered by Kiwix" should not be part of the widget IMO. I continue with my testing....

kelson42 · 2022-08-15T09:45:20Z

Modulo the remarks done earlier, this first version of the new /widget feature looks good to me. Adding @veloman-yunkan for the code review

juuz0 · 2022-08-15T12:29:38Z

I see nothing which is different than without custom CSS. Considering that there is nothing which has the DOM id="receiver", I suspect the documentation to be simply wrong or incomplete.

The documentation does have the id=receiver in iframe, only in CSS/JS section :)
There are two mistakes in the code you sent:

id=receiver in iframe
receiver.postMessage(msg, <URL here>); and <iframe src="URL here" width=1000 height=1000></iframe> should have the same URL (I should probably mention it clearly in docs)
Here's the fixed version:

<!DOCTYPE html>
<html lang="en">
<head>
<title>Widget Test</title>
</head>
<body>
  <iframe id="receiver" src="http://localhost:8181/widget" width=1000 height=1000></iframe>

  <script>
window.onload = function() {
var receiver = document.getElementById('receiver').contentWindow;
function sendMessage() {
    let msg = {
    css: `
    .book__header {
        color:red;
    }`,
    js: `
    function widgetTest() {
    console.log("Testing widget");
    }
    widgetTest();
    `
    }
    receiver.postMessage(msg, 'http://localhost:8181/widget');
}
sendMessage();
}
</script>

</body>
</html>

Adds handling for parameters: disablefilter - disable search filters disableclick - disable book click action disabledownload - disable download button disabledesc - disable description

Added an event listener for message event. The idea is the website which embeds the widget will send a message using postMessage(). The expected message is: { css: // custom CSS code js: // custom JS code }

Added documentation for current widget usage, currently supported arguments, using custom CSS/JS

juuz0 · 2022-08-15T12:57:54Z

An other detail, "Powered by Kiwix" should not be part of the widget IMO

Done.
Also the commit which provided a way to filter books by name (should be done in C++ side in other PR)

kelson42 · 2022-08-15T13:11:16Z

@juuz0 Please create a dedicated ticket + PR for selecting ZIM files based on there ZIM Name metadata. It should work in general and not only for the widget.

juuz0 · 2022-08-15T13:49:58Z

@kelson42 #807

veloman-yunkan · 2022-08-15T18:40:47Z

static/skin/index.js

-            document.querySelector(nodeQuery).innerHTML += optionStr;
+            const entryList = data.querySelectorAll('entry');
+            const nodeQueryElem = document.querySelector(nodeQuery);
+            if (entryList && nodeQueryElem) {


When nodeQueryElem == null doesn't the loadAndDisplayOptions() function become a no-op? If so why run fetch(query) at all?

veloman-yunkan · 2022-08-15T18:45:37Z

docs/widget.rst

+disableclick (value = N/A)
+    Disables clicking the book to open it for reading.


For me (Firefox 103.0) a blank tab is opened

veloman-yunkan · 2022-08-15T18:55:36Z

static/skin/widget.js

+function disableSearchFilters(widgetStyles) {
+    const hideNavRule = `
+    .kiwixNav {
+        display: none;
+    }`;
+    const hideResultsLabelRule = `
+    .kiwixHomeBody__results {
+        display: none;
+    }`;
+    const hideTagFilterRule = `
+    .book__tags {
+        pointer-events: none;
+    }`;
+    insertNewCssRules(widgetStyles, [hideNavRule, hideResultsLabelRule, hideTagFilterRule]);
+}


I think that if you introduce custom CSS handling first, then disableSearchFilters(), disableDownload(), disableDescription() and hideFooter() can be rewritten via addCustomCss() in a simpler way.

kelson42 · 2022-08-24T14:43:57Z

@juuz0 Any feedback?

stale · 2022-11-02T04:45:59Z

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.

juuz0 requested a review from kelson42 July 16, 2022 17:02

juuz0 force-pushed the widgetEndpoint branch 3 times, most recently from d279aff to fb3eaf5 Compare July 28, 2022 21:38

juuz0 marked this pull request as ready for review July 29, 2022 16:08

juuz0 force-pushed the widgetEndpoint branch from fb3eaf5 to 602b807 Compare August 1, 2022 14:27

kelson42 changed the title ~~add widget support~~ Add widget support Aug 6, 2022

kelson42 force-pushed the widgetEndpoint branch from 602b807 to f42d773 Compare August 6, 2022 05:40

juuz0 added 3 commits August 11, 2022 21:10

Give a name to function for updating book count

489dfc1

Extracts function updateBookCount() from the unnamed function in resize event.

Add widget endpoint

efe42c9

Adds an endpoint /widget to provide kiwix serve widget.

Give name to IIFE in index.js and expose updateBookCount

d27220f

Gives a name to the IIFE wrapping code in index.js - kiwixServe and exposes updateBookCount through it

juuz0 force-pushed the widgetEndpoint branch from f42d773 to 282e36c Compare August 11, 2022 15:50

kelson42 requested changes Aug 13, 2022

View reviewed changes

kelson42 requested review from kelson42 and veloman-yunkan August 15, 2022 09:44

kelson42 mentioned this pull request Aug 15, 2022

Introduce extension:iframe offspot/mediawiki-docker#35

Closed

juuz0 added 2 commits August 15, 2022 18:21

Basic widget handling

58c04b3

Adds handling for parameters: disablefilter - disable search filters disableclick - disable book click action disabledownload - disable download button disabledesc - disable description

Add custom CSS and JS support

09dc6f9

Added an event listener for message event. The idea is the website which embeds the widget will send a message using postMessage(). The expected message is: { css: // custom CSS code js: // custom JS code }

Add documentation for widget

fc87def

Added documentation for current widget usage, currently supported arguments, using custom CSS/JS

juuz0 force-pushed the widgetEndpoint branch from 282e36c to fc87def Compare August 15, 2022 12:52

veloman-yunkan reviewed Aug 15, 2022

View reviewed changes

Jaifroid mentioned this pull request Sep 4, 2022

Fix Library so that required archives can be downloaded from non-mirrored directories kiwix/kiwix-js-pwa#288

Closed

stale bot added the stale label Nov 2, 2022

		disableclick (value = N/A)
		Disables clicking the book to open it for reading.

Uh oh!

Conversation

juuz0 commented Jul 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jul 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

juuz0 commented Jul 16, 2022

Uh oh!

mgautierfr commented Jul 19, 2022

Uh oh!

kelson42 commented Jul 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juuz0 commented Jul 28, 2022

Uh oh!

kelson42 commented Aug 1, 2022

Uh oh!

juuz0 commented Aug 1, 2022

Uh oh!

juuz0 commented Aug 6, 2022

Uh oh!

kelson42 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juuz0 commented Aug 14, 2022

Uh oh!

kelson42 commented Aug 14, 2022

Uh oh!

kelson42 commented Aug 15, 2022

Uh oh!

juuz0 commented Aug 15, 2022

Uh oh!

juuz0 commented Aug 15, 2022

Uh oh!

kelson42 commented Aug 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

juuz0 commented Aug 15, 2022

Uh oh!

veloman-yunkan Aug 15, 2022

Choose a reason for hiding this comment

Uh oh!

veloman-yunkan Aug 15, 2022

Choose a reason for hiding this comment

Uh oh!

veloman-yunkan Aug 15, 2022

Choose a reason for hiding this comment

Uh oh!

kelson42 commented Aug 24, 2022

Uh oh!

stale bot commented Nov 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

juuz0 commented Jul 16, 2022 •

edited

Loading

codecov bot commented Jul 16, 2022 •

edited

Loading

kelson42 commented Jul 20, 2022 •

edited

Loading

kelson42 left a comment •

edited

Loading

kelson42 commented Aug 15, 2022 •

edited

Loading