-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Migrate to Interstitial for Trusted Book Provider read actions #10586
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
Migrate to Interstitial for Trusted Book Provider read actions #10586
Conversation
…ction, update read button templates
for more information, see https://pre-commit.ci
openlibrary/templates/book_providers/standard_ebooks_read_button.html
Outdated
Show resolved
Hide resolved
There were a couple URLs that I wasn't sure behaved the same way as the standard read/borrow action URls that I haven't touched, those being the librivox template (because it's an audio link) and the direct provider preview link (uses the same URL as the regular read link did but not sure if we want the same behavior?) |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
openlibrary/templates/book_providers/standard_ebooks_read_button.html
Outdated
Show resolved
Hide resolved
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.
The Interstitial design / presentation needs a bit of work; some suggestions provided
Marking as ready for review according to discussion from the community meeting. For some reason I am unable to get the template CSS to work on my end, but the lines that could be added optimally are |
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.
Thanks @SivanC!
I left some review notes, and am happy to merge this once those are addressed.
The main issues are:
- Interstitial page needs some margins
- Some
aria
attributes are no longer appropriate without toast messages - JS in template should be moved to Webpack (cancel link code is optional)
openlibrary/templates/book_providers/cita_press_read_button.html
Outdated
Show resolved
Hide resolved
|
||
<p> | ||
<a href="#" onclick="window.close(); return false;">$_("Cancel")</a> | |
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 isn't necessary for this PR, but noting that we may want to move this into Webpack in the future. This could be a good first issue, and the solution could be modeled on our recent cancel link changes (#9795).
Definitely not a blocker.
@jimchamp Thanks for the detailed review! I'll take a crack tomorrow, haven't dived too deep into the JS side of things on the project so might shoot you some questions if I get stuck somewhere |
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.
Thanks for the quick turn-around, @SivanC! I'm going to make a small commit to remove my unhelpful comments, then merge. This is great!
6978961
into
internetarchive:trustedbookprovider-interstitial
Closes #10561
Feature/refactor
Technical
Add new behavior to borrow API to redirect to an interstitial page in the case that a read/borrow action is requested on a non-IA provided edition. Also removes the existing toasts that are generated when using a direct provider read button link.
Testing
Screenshot
Stakeholders
@mekarpeles