Skip to content
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

Fix: Foreign Affairs update front-end. #3407

Merged
merged 9 commits into from
Jan 21, 2025
Merged

Conversation

Wenzhi-Ding
Copy link
Contributor

@Wenzhi-Ding Wenzhi-Ding commented Jan 8, 2025

Recently, Foreign Affairs changed its front-end code, so the previous translator no longer works. See test pages:

http://www.foreignaffairs.com/issues/2012/91/01
https://www.foreignaffairs.com/reviews/capsule-review/2003-05-01/history-argentina-twentieth-century
https://www.foreignaffairs.com/search/argentina

I have updated the translator to accommodate the new layout for all these three types of pages. All tests are passed. Could you kindly review the pull request and merge it if it's fine? Many thanks!

I have posted this issue on the Zotero forum: https://forums.zotero.org/discussion/120979/translator-for-foreign-affairs-needs-update

Copy link
Member

@AbeJellinek AbeJellinek left a comment

Choose a reason for hiding this comment

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

Thanks, looks good! A few comments.

Foreign Affairs.js Outdated Show resolved Hide resolved
Foreign Affairs.js Outdated Show resolved Hide resolved
Foreign Affairs.js Outdated Show resolved Hide resolved
let authors = author.split(/, and|and |, /);
for (let aut of authors) {
item.creators.push(ZU.cleanAuthor(aut, "author"));
}

var tags = doc.querySelectorAll('.mr-15');
Copy link
Member

Choose a reason for hiding this comment

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

Not a reliable selector (this is a Tailwind margin style) - anything else we can match on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to:

	var tags = doc.querySelectorAll(
		'a[href^="/regions/"].text-decoration-none, ' +
		'a[href^="/topics/"].text-decoration-none, ' +
		'a[href^="/tags/"].text-decoration-none'
		);

Copy link
Member

Choose a reason for hiding this comment

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

This is still matching on a Tailwind class, so still unstable. Anything else we could use? Do we need the class in the selector at all?

Copy link
Member

Choose a reason for hiding this comment

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

Like:

var tags = doc.querySelectorAll(`
	ul > li > a[href^="/regions"],
	ul > li > a[href^="/topics"],
	ul > li > a[href^="/tags"]
`);

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comment. I now change to the style as suggested.

	var tags = doc.querySelectorAll(
		'main > div > div > div > div > ul > li > a[href^="/regions/"], ' +
		'main > div > div > div > div > ul > li > a[href^="/topics/"], ' +
		'main > div > div > div > div > ul > li > a[href^="/tags/"]'
		);

Copy link
Member

Choose a reason for hiding this comment

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

That isn't really what I suggested, but I see that what I suggested would've matched non-tag links. Just pushed an update to fix that.

@AbeJellinek AbeJellinek merged commit d4e43e2 into zotero:master Jan 21, 2025
1 check failed
@AbeJellinek
Copy link
Member

Thanks!

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