search

readium / readium-js-viewer

👁 ReadiumJS viewer: default web app for Readium.js library
BSD 3-Clause "New" or "Revised" License
550 stars 186 forks source link

Deprecation message in Chrome app #713

Closed danielweck closed 5 years ago

danielweck commented 5 years ago

Fixes https://github.com/readium/readium-js-viewer/issues/712

Note that the same nagging dialog pops up every time the library view is loaded. The user cannot tick a "stop nagging me" checkbox. The reading system does not limit the number of times the message is shown. There is no notion of reading "session" or time window during which the message should only be shown x number of times.

Test URL (library view): http://readium-chromeapp.surge.sh/?epubs=https%3A%2F%2Freadium.firebaseapp.com%2Fepub_content%2Fepub_library.opds

Test URL (reader view, no nagging message): http://readium-chromeapp.surge.sh/?epub=https%3A%2F%2Freadium.firebaseapp.com%2Fepub_content%2Faccessible_epub_3

Note that this Pull Request enables the nagging dialog for the cloud reader as well as the Chrome app, so that we can test this feature easily (using the above URLs). There is a boolean in the proposed code to enable this feature selectively, i.e. only for the Chrome app in the Chrome web browser, but not in Chrome OS.

rkwright commented 5 years ago

@danielweck This is excellent. Much better than my hacky approach. But I am not sure that we need to remind them every time they open a book. I would think that each time when the library view is shown would be sufficient.

danielweck commented 5 years ago

Sounds good to me. I forgot that in the Chrome App there is no way to open an EPUB directly (no direct URL to a specific publication). The UX always lands the user in the library view.

danielweck commented 5 years ago

Okay, I removed the nagging message in the reader view.

danielweck commented 5 years ago

Note that I do not have a Chrome Book (Chrome OS), so I am unable to verify that the nagging message only appears in the Chrome app (in the Chrome web browser)

danielweck commented 5 years ago

@rkwright once the i18n file is fixed (rollback the specific line change I commented on), then I am happy to merge. Thanks for your tweaks! :)

rkwright commented 5 years ago

@danielweck Fixed in https://github.com/readium/readium-js-viewer/commit/ce994f5be8362b65bf9571c12b7ebac67a5c327a

danielweck commented 5 years ago

LGTM, LGTM (Looks Good To Me, Let's Get This Merged) ;)