-
-
Notifications
You must be signed in to change notification settings - Fork 56
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
Get correct href value on onClick
for "warc2zim" files.
#1036
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1036 +/- ##
=======================================
Coverage 38.95% 38.95%
=======================================
Files 58 58
Lines 3958 3958
Branches 2181 2181
=======================================
Hits 1542 1542
Misses 1086 1086
Partials 1330 1330 ☔ View full report in Codecov by Sentry. |
As I will need to keep up with changes on the reader side, I need to understand the effect of this. I had noticed that getting the |
At the trustiest place : source code of wombat. [*] I am NOT saying there isn't. Just I'm not aware of.
I think so. Another solution could be to do |
Thanks. I tried many things, and |
Just wanted to say "big thank you" 🙏 for finding the I'm going to open an issue in the Wombat source Repo to add at least that bit of documentation! |
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.
LGTMBIPTMISB*
* Looks good to me, but I propose to make it slightly better
static/skin/viewer.js
Outdated
if ( isExternalUrl(target.href) ) { | ||
const possiblyBlockedLink = blockLink(target.href); | ||
// In case of wombat in the middle, wombat will rewrite the href value to the original url (external link) | ||
// This is not what we want. Let's ask wombat to not rewrite href | ||
const old_no_rewrite = target._no_rewrite; | ||
target._no_rewrite = true; | ||
const target_href = target.href; | ||
target._no_rewrite = old_no_rewrite; | ||
if (isExternalUrl(target_href)) { | ||
const possiblyBlockedLink = blockLink(target_href); |
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.
Please encapsulate (part of) this change in a helper function get????Href()
. The hard part is figuring out what to put instead of ????
in the function name.
db76ace
to
d66d28d
Compare
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.
Fixup commit must be squashed
Next to come warc2zim archive will come with "wombat" embedded. The purpose of wombat is to be an interface with js code to mask that we are in a scrapped/zim context to the js. So it rewrite the `.href` attributes to the original url (ie, an absolute url to the original website), even if the local relative url is valid. Let's ask to wombat to not rewrite href in our special case.
d66d28d
to
9375f97
Compare
Next to come warc2zim archive will come with "wombat" embedded. The purpose of wombat is to be an interface with js code to mask that we are in a scrapped/zim context to the js.
So it rewrite the
.href
attributes to the original url (ie, an absolute url to the original website), even if the local relative url is valid.Let's ask to wombat to not rewrite href in our special case.