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

Refactor javascript code for fallback thumbnail, use something more simplier #1521

Merged
merged 3 commits into from
Mar 4, 2020

Conversation

murny
Copy link
Contributor

@murny murny commented Mar 4, 2020

So this was an issue I was fighting with for awhile on the webpacker PR.

The problem is we want this default_thumbnail() method setup as a global method and exposed to the entire app (e.g you can call it from browser console, or any html). This goes against Webpackers defaults and is a major pain to get something like this to work in webpacker. I tried to replace this with jquery and watching for .error/.load events on the image but it works half the time (works on turbolinks page loads, but on a hard refresh it fails as image fails to load before javascript loads and fires). Also looked into using a library like https://github.com/desandro/imagesloaded but didn't get much success either. Other option is opening a <script></script tag and throwing this function inside to by pass webpacker...but thats messy as well.

Instead of spending a ton of time on this, I just took a simple approach. Just fallback to the ERA logo without any text on it. We do this for community thumbnails already, so figured this be okay here as well.

Tested this and it all works quite nicely. If we cannot load an image we just get the ERA logo without text as such:

image

@@ -1,8 +0,0 @@
// Replace thumbnail with default thumbnail on error
function default_thumbnail($element) {
Copy link
Contributor Author

@murny murny Mar 4, 2020

Choose a reason for hiding this comment

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

Some future feedback on this javascript.

It's a best practice to create variables with $ in them, when they are jquery selectors.

So for example, const $imageSelector = $('img'); is a good practice. Then by quickly looking at the variable you know if you can use Jquery methods on them or not!

But these $element and $thumbnail_html variables are not a jquery selector, so this may cause confusion if you expect them to behave as such

ConnorSheremeta
ConnorSheremeta previously approved these changes Mar 4, 2020
mbarnett
mbarnett previously approved these changes Mar 4, 2020
@murny murny dismissed stale reviews from mbarnett and ConnorSheremeta via 03f3ee4 March 4, 2020 21:22
@murny murny merged commit 2bd45b0 into integration_postmigration Mar 4, 2020
@murny murny deleted the refactor-fallback-thumbnail branch March 4, 2020 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants