Widget endpoint: /widget Fixes https://github.com/kiwix/libkiwix/issues/585

Jul 16 '22 16:07 juuz0

Codecov Report

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

:exclamation: 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%)`	:arrow_up:

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

Jul 16 '22 16:07 codecov[bot]

Will fix the code coverage :+1: 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

Jul 16 '22 17:07 juuz0

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)

Jul 19 '22 14:07 mgautierfr

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

Jul 20 '22 17:07 kelson42

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

Jul 28 '22 21:07 juuz0

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

Aug 01 '22 13:08 kelson42

@kelson42 Done.

Aug 01 '22 14:08 juuz0

@kelson42 Any updates here? :)

Aug 06 '22 14:08 juuz0

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.

Aug 14 '22 16:08 juuz0

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

Aug 14 '22 17:08 kelson42

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

Aug 15 '22 09:08 kelson42

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>

Aug 15 '22 12:08 juuz0

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)

Aug 15 '22 12:08 juuz0

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

Aug 15 '22 13:08 kelson42

@kelson42 #807

Aug 15 '22 13:08 juuz0

@juuz0 Any feedback?

Aug 24 '22 14:08 kelson42

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.

Nov 02 '22 04:11 stale[bot]

Add widget support

Codecov Report