Added linkPreviewsToCovers configuration setting. #762
Conversation
| @@ -43,7 +43,7 @@ function applyPreviewUrl($link, url) { | |||
|
|
|||
| // Update associated record thumbnail, if any: | |||
| $link.parents('.result,.record') | |||
| .find('.recordcover').parents('a').attr('href', url); | |||
| .find('.recordcover[data-linkpreview="true"]').parents('a').attr('href', url); | |||
There was a problem hiding this comment.
Using a class-based search here (make the pictures .recordcover.linkpreview instead of a data attribute) may be more performant. It may only be a difference of tens of milliseconds, if you think this is much clearer syntactically.
There was a problem hiding this comment.
My gut feeling is that it's smarter to use data-elements instead of classes when we're just trying to flag things, since it reduces the chances of unintended CSS-related side effects. If the performance impact were significant, I could of course be persuaded to use a class here, but if it's just a matter of ms, I think clarity and lack of side effects are worth the cost. On the other hand, if you think there might be a situation where somebody would WANT to style things differently based on this class, that could be another argument for using classes... except that this is just a flag class, and it doesn't tell us whether or not a preview has actually been linked, it just means that one might be. If we wanted a stylistic indication of which covers are linked to previews, we would have to apply an additional class in preview.js... so I think that's really a separate matter entirely.
So bottom line, what do you think? Do you agree that it's okay as-is, or do you lean toward the class approach over the data attribute?
There was a problem hiding this comment.
I guess we have no need to squabble over milliseconds. And if someone wanted to style it they could use the same selector. Fine as is.
There was a problem hiding this comment.
Good point regarding selectors -- nothing protects us completely against side effects. :) In any case, I'll go ahead and merge!
Currently, when the preview setting is turned on, all thumbnail links are replaced with links to third-party book previews. This is not always desirable. This pull request adds a config setting which allows preview linking to be either completely disabled, or else configured on a page-by-page basis.