-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
add Wikipedia links from Wikidata #9991
Conversation
Dropping a bunch of profiles here we decided to leave out for the first version.
|
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 code looks good overall; I can't quite test it until the http proxy is set up on our servers so that they can make the wikidata network call. That should be happening in the next day or so (CC @jimchamp ).
[ | ||
{ | ||
"url": f"{profile_config['base_url']}{value}", | ||
"icon_url": f"/static/images/identifier_icons/{profile_config["icon_name"]}", |
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.
"icon_url": f"/static/images/identifier_icons/{profile_config["icon_name"]}", | |
"icon_url": f"/static/images/identifier_icons/{profile_config['icon_name']}", |
?
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.
In python 3.12+, this is actually ok! They rewrote the parser to make f-strings more resilient :) https://peps.python.org/pep-0701/
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.
Oooh. Neat!
@@ -32,14 +42,10 @@ class WikidataEntity: | |||
labels: dict[str, str] | |||
descriptions: dict[str, str] | |||
aliases: dict[str, list[str]] | |||
statements: dict[str, dict] | |||
statements: dict[str, list[dict]] | |||
sitelinks: dict[str, dict] | |||
_updated: datetime # This is when we fetched the data, not when the entity was changed in Wikidata |
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.
I know this is from a different PR, but would it make sense to call this _fetched
or _last_fetched
instead to avoid the ambiguity and need of the comment? (Probably also something for a different PR since, either way, it’s unrelated to adding Wikipedia links.)
$def render_social_icon(url, icon_url, title): | ||
<a href="$url" title="$_('View on %(site)s', site=title)"> | ||
<img class="profile-icon" src="$icon_url" alt=""> |
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.
$def render_social_icon(url, icon_url, title): | |
<a href="$url" title="$_('View on %(site)s', site=title)"> | |
<img class="profile-icon" src="$icon_url" alt=""> | |
$def render_social_icon(url, icon_url, label): | |
<a href="$url" title="$_('View on %(site)s', site=label)"> | |
<img class="profile-icon" src="$icon_url" alt="$label"> |
The alt
property should describe/be a stand‐in for the image, so the social’s label
is probably a good fit. (Also, changing the title
to label
to be consistent with how it’s named in the dict, since my first iteration of this had the function signature take an addition label
property…)
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.
In this case, because we have the title
tag on the above anchor element, the image can be seen as purely presentational, and doesn't require an alt tag (or more specifically requires an alt=""
tag to denote it's presentational). E.g. screen readers will read the title
tag, I believe. But this is a bit of an edge case, so not entirely sure.
@@ -14,6 +14,11 @@ | |||
<td><span itemprop="$itemprop">$value</span></td> | |||
</tr> | |||
|
|||
$def render_social_icon(url, icon_url, title): | |||
<a href="$url" title="$_('View on %(site)s', site=title)"> |
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.
<a href="$url" title="$_('View on %(site)s', site=title)"> | |
<a href="$url" title="$_('View on %(site)s', site=title)" data-ol-link-track="AuthorSocialLink|$title"> |
For another PR: it would be nice to have analytics on this; can $title have spaces/etc we might need to strip?
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.
Ok this is now working with @jimchamp 's latest PR changes ! 🥳
El mié., 20 de noviembre de 2024 11:17 a. m., Drini Cami <
***@***.***> escribió:
… ***@***.**** commented on this pull request.
------------------------------
In openlibrary/templates/authors/infobox.html
<#9991 (comment)>
:
> + <a href="$url">
+ <img class="profile-icon" src="$icon_url" title="$title">
⬇️ Suggested change
- <a href="$url">
- <img class="profile-icon" src="$icon_url" title="$title">
+ <a href="$url" title="$_('View on %(site)s', site=title)">
+ <img class="profile-icon" src="$icon_url" alt="">
I think the title is more common <a> since that helps screen readers.
Then on the image, you can set alt="" to make the image essentially
presentation, since the actual info is the title (+ remove the title from
the image).
—
Reply to this email directly, view it on GitHub
<#9991 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BHO4HIRQ6RSAPRQEOTTLQF32BS72FAVCNFSM6AAAAABRFYVIVGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDINBZGI3DGMBQHA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Closes #9360
This PR does three things:
Originally, this was just going to be for Wikipedia but I quickly realized the code should be cleaned up and ready to handle multiple profiles since that's the main idea. We settled on Wikipedia because it's the most useful, Wikidata so it can point people where to edit, and Google Scholar because it's well known and only really applies to academics. It's a nice first step before we discuss what icons we want to show and the order.
Technical
Testing
Screenshot
Stakeholders
@cdrini