Skip to content

Conversation

robertsky
Copy link
Contributor

simplify byline detection for straits times. update author list

@AbeJellinek
Copy link
Member

Could you add a search test? I'm having trouble finding a search page that this detects (e.g. https://www.straitstimes.com/search?searchkey=test doesn't work).

if (url.includes('/search?') && url.includes('searchKey')) {
Zotero.debug(url);
if (url.includes('/search?') && url.includes('searchkey')) {
await sleep(1000);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AbeJellinek apparently the third party tool that the site uses for search loads much slower, hence the sleep here. but i am not sure if this is the right way to go about it... Also when I tried to run and update the test, it went into a perpetual 'updating' mode?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You won't have access to setTimeout() in all translation environments, so we can't do that. More importantly, though, doc is a static copy of the document in its current state; it won't change (in the browser) when the page updates.

The right way to do this is by calling ZU.monitorDOMChanges() in detectWeb(). Take a look at the standard web translator template for an example of what this should look like:

https://github.com/zotero/zotero/blob/cee1e3a596a7ecb383beb46050990b11cc1eb59b/chrome/content/scaffold/templates/newWeb.js#L25-L65

The only difference here is that if the URL matches a search page but getSearchResults(doc, true) returns false, we'll want to call ZU.monitorDOMChanges(someTargetElement), which will automatically rerun detectWeb() when that element changes.

(Right now we're only matching search pages with the search term test, so that will also need to be changed.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants