-
-
Notifications
You must be signed in to change notification settings - Fork 135
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
Adding browser for zim library #1127
Conversation
Yes, this looks good to me! It might be a good idea for a new tab to appear at the top when the user presses the button, if that's possible (i.e. add one programmatically, that would disappear when leaving the page), but that's a detail that could be left till after implementation. |
You're right, no point adding it till #523, unless it's really clear that's going to take a very long time. |
@Jaifroid could you take a quick look I have added a library with working app theme |
It's looking good! Regarding download, on each tile there is a small, narrow strip that says "Download" -- easy to miss. You have to click it, and not anywhere else on the tile, which is a bit counterintuitive IMHO, but it is what it is. It'a an accessibility nightmare, but not our problem directly... I also find it problematic that the filename of the archive is only visible (in some browsers) by hovering over the tile and looking at any link preview the browser might provide. It really should be on each tile, and I've argued for this, but for some reason I really cannot fathom there is resistance to the idea... It's really difficult to distinguish amongst the many different Wikipedia offerings for this reason. Again, not our problem, directly, and patching the incoming code would create a maintenance issue. |
Two small things:
|
Ah, and clicking the download button leads to content being blocked by the sandbox, see below. To fix that, you'll have to add an onclick event to the iframe and check what's being clicked, redirect it to top-level scope so the download occurs outside the iframe. This is the same trick we use to download external links from inside a ZIM. Look for that code and if possible re-use it. |
Look for code Hopefully the server permits CORS, so you can inspect the clicked link. If it's blocked, we'll need to ask the library.kiwix.org devs to add a CORS permissive header (I had to ask for this in the case of download.kiwix.org). |
Final thing before I forget is that library.kiwix.org doesn't support IE11 or Edge Legacy. So I suggest when the user clicks the open library button, you should sniff for a function that only those two browsers support -- I use I suggest defining a parameter in |
Yes, that's why I said we might have to ask library.kiwix.org to set a CORS-permissive header. It looks like it. |
Can you tell me what exactly i have to do now? |
Need to check what header it is returning currently, and compare to the header returned by download.kiwix.org. You should be able to check this in the network tab of browser. Check for the CORS headers in download.kiwix.org. It's possible even CORS permissions won't fix it, because in my code for download.kiwix.org, I get the raw HTML and then manipulate that before injecting it into a DIV. I was hoping we could avoid that with library.kiwix.org... You could try switching to |
Okay it fixed it, but do you mind telling me how can I get all the mirrors |
How did you fix it on your side? Regarding mirrors, this should be handled automatically when you click a download link in library.kiwix.org (or in download.kiwix.org). They use mirrorbrain software server-side, so it should redirect to a mirror automatically. But it depends what you're doing with the download link of course! |
|
OK, well done! So it was our sandbox rather than server-side lack of CORS policy. Well spotted. |
On mirror source, I guess you'd have to experiment to see what works, it's not very transparent exactly how it works. |
I think you can review PR now |
I'll need a bit of time -- I've got some very urgent deadlines... |
ok no worries
I have pushed a solution and I am 90% confident this will be it. I have allowed every subdomain for the mirror domain |
Just quickly (without reviewing properly), I notice you haven't internationalized the new button text. Please add: Spanish: "Biblioteca ZIM" (I don't think we need the word for "browse" in each case, as the buttons will become too big). Look at how it's done for the adjacent button (data-i18n attribute), and then add the appropriate strings in Then test buttons in all three languages and on narrow screen sizes to ensure responsiveness. Thanks! |
Happy to confirm going with the suggested path, and thanks for further explanation. So, I suggest that if it is Chrome in a local extension, which basically means Chrome in jQuery mode OR in localServiceWorker mode, we should open Let me know if you don't follow that hasty explanation, or if I got anything wrong. |
PS I don't think you need to be too elaborate: if it's easier just to detect that library.kiwix.org won't work, and in all such cases just open a new tab with download.kiwix.org (whether it's IE11, Edge Legacy, or Chrome in a local extension), then that would be fine too. |
fad28f8
to
de8b510
Compare
Update: I've implemented the XHR method, but unfortunately, it's not functioning correctly in IE11. When clicking on a link, IE opens the file as text instead of downloading it. I've tried various approaches, like opening a new tab for the URL and setting the download URL from the app root, but none of them seem to work. I'm starting to think it might be best to take a different approach: when the platform is not supported, we could simply open the link in a new tab. Additionally, I've been working on this PR for a while, and I'm concerned that it might not be adding significant value to the organization. If it's acceptable, we could proceed with the above approach for now and revisit this PR later to add support for more versions. |
I'm very happy for you to open in a new tab when library.kiwix.org is not supported. In fact it's what I though you'd decided to do in your response to #1127 (comment). XMLHttpRequest does work in IE11 for most mirrors, but not all mirrors, because some do not send the right headers to force a download in the browser. Maybe you were stuck testing a mirror that doesn't work. In any case, that comment is not implying you should try to fix it. As we discussed above, opening |
Interesting! I'll take a look. And yes, please feel free to start work on #1032. I'll add some thoughts in that issue. |
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.
This is looking excellent to me! Well done, it makes a big difference to UX. You'll see I have some very minor suggestions that will probably take 5 or 10 minutes. While you're doing that, I'd like to check the code in an old Firefox OS IDE, which is also a good way of checking that there aren't any regressions with old browsers. So let me know when you've done the minor changes, so I can confirm I've completed that test.
OK, the Firefox OS test has thrown up a small issue, which is that the file selectors are hidden in that implementation (see screenshot), and unfortunately that includes the library button. It's because in Firefox OS there is access to the FS, so the app scans the FS instead of relying on file pickers. The library button is currently hidden along with the standard file picker. |
Or else, we could say that some reworking of the UI will be necessary for #656, so we could deal with it there. Any thoughts? |
When clicking on the button, I get the screen below. Any idea why it's untrusted? Are you using https? In any case, the user can click on "I understand the risks", and then the library opens, so I think that's acceptable. But we should just check that it's not redirecting to http: instead of https:... |
And the good news is that it's possible to download a ZIM, though the user has to go through another warning screen. Tbh, I think it's just the age of the device (it's kind of a miracle it works at all, really). NB I support this because Kiwix JS was originally built for Firefox OS, and the previous main developer was very attached to it. It's a good proxy for possible regressions. But I don't expect it to function perfectly any more. |
Yes, let's merge it here first I will figure it out later
All the URLs are https, not sure why it's giving a warning |
OK, so I think it's just the age of the device, probably doesn't handle https very well. At least it gives the option to go ahead anyway. So, when you're happy with your final fixes (let me know if you want me to check anything), go ahead and squash/merge. |
closes #846
kiwix.mp4
This kind of setup seems best to me if it a easily visible if he wants to open the library + a new user doesn't have to look anywhere