New plugin: theguardian.com#1782
Conversation
Load thumbnails and the pictures inside articles. Signed-off-by: Bitterblue Smith <rtl8821cerfe2@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request adds support for zooming images on The Guardian by introducing a new plugin (plugins/theguardian.js) and registering it in manifest.json. Feedback on the implementation includes correcting a syntax error in a jQuery selector (missing closing bracket), adding a defensive check to prevent a potential TypeError when an image is not found, and a suggestion to simplify the entire plugin by using hoverZoom.urlReplace for all images, which would also resolve hover area issues.
|
|
||
| // This handles article thumbnails on the main page, and images | ||
| // inside articles. | ||
| $('a[href^="/"], a[href^="#img"], a[href*="theguardian.com"').one('mouseenter', function () { |
There was a problem hiding this comment.
There is a syntax error in the jQuery selector: 'a[href*="theguardian.com"' is missing its closing bracket ']'. It should be 'a[href*="theguardian.com"]'.
| $('a[href^="/"], a[href^="#img"], a[href*="theguardian.com"').one('mouseenter', function () { | |
| $('a[href^="/"], a[href^="#img"], a[href*="theguardian.com"]').one('mouseenter', function () { | |
| var img = a.parent().find('img[src*="i.guim.co.uk/"]'); | ||
| var url = img.get(0).src; |
There was a problem hiding this comment.
If no matching 'img' element is found, 'img.get(0)' will be 'undefined', and attempting to access '.src' will throw a 'TypeError', crashing the event handler. You should add a defensive check to ensure the image element exists before accessing its properties.
| var img = a.parent().find('img[src*="i.guim.co.uk/"]'); | |
| var url = img.get(0).src; | |
| var img = a.parent().find('img[src*="i.guim.co.uk/"]'); | |
| if (!img.length) { | |
| return; | |
| } | |
| var url = img.get(0).src; |
| // This handles article thumbnails on the main page, and images | ||
| // inside articles. | ||
| $('a[href^="/"], a[href^="#img"], a[href*="theguardian.com"').one('mouseenter', function () { | ||
| var a = $(this); | ||
| var data = a.data(); | ||
|
|
||
| if (data.hoverZoomSrc) { | ||
| return; | ||
| } | ||
|
|
||
| var img = a.parent().find('img[src*="i.guim.co.uk/"]'); | ||
| var url = img.get(0).src; | ||
|
|
||
| data.hoverZoomSrc = [url.replace(old_width, new_width)]; | ||
|
|
||
| a.addClass('hoverZoomLink'); | ||
| hoverZoom.displayPicFromElement(a); | ||
| }); | ||
|
|
||
| // This handles the profile picture of the article's author. | ||
| // The width parameter is necessary. It doesn't accept | ||
| // ridiculously large numbers, like 2100 and larger, but 2000 | ||
| // seems fine. | ||
| hoverZoom.urlReplace(res, | ||
| 'img[src*="i.guim.co.uk/"]', | ||
| old_width, | ||
| new_width, | ||
| ); |
There was a problem hiding this comment.
To solve the issue you mentioned in the PR description (where hovering anywhere inside the large blue link rectangle triggers the zoom instead of just the thumbnail), you can target the 'img' elements directly rather than the wrapping 'a' elements.
HoverZoom's 'urlReplace' utility already handles image elements perfectly and binds the hover event directly to them. Since the article thumbnails and the author profile pictures both use the 'i.guim.co.uk' domain, you can simplify the entire plugin by using 'urlReplace' for all of them. This removes the need for the manual 'mouseenter' event listener entirely and solves your hover area issue.
// This handles all images hosted on the Guardian's CDN (i.guim.co.uk),
// including article thumbnails, images inside articles, and author profile pictures.
hoverZoom.urlReplace(res,
'img[src*="i.guim.co.uk/"]',
old_width,
new_width
);
Load thumbnails and the pictures inside articles.
This works, but there is a problem: it loads the thumbnail associated with a link when hovering anywhere inside the blue rectangle:
I don't know how to make it load the thumbnail only when hovering over the thumbnail. I tried to select the
<img>tags instead of<a>tags, but the mouseenter event doesn't fire.This is why I'm making this a draft pull request. Maybe someone has a solution?