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

thumbnailPath function can now signal to revert to default functionality #363

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ericnewton76
Copy link

@ericnewton76 ericnewton76 commented Apr 8, 2022

I ran into an issue, where I'm loading Video preview urls from (youtube/vimeo) and self hosted Images that have an endpoint similar to Flikr. I was using sizeRangeSuffixes to rewrite the self hosted images but this was also applying to the Preview images hosted by youtube/vimeo which dont support this.

The goal was to provide a function to thumbnailPath to just handle videos (leave them alone) and apply sizeRangeSuffixes to all the other images.

In the process, I found a bug where this.resetImgSrc was undefined inside the error handler due to jQuery rewriting what this is.

Example usage:

thumbnailPath: function(src, w, h, img) {

  if($(img).data("isVideo") == "true") {
    return src;
  else
    return null; //fallback to default handling that can use sizeRangeSuffixes

}

@ericnewton76
Copy link
Author

ericnewton76 commented Apr 8, 2022

BTW tried to run npm test but it errors with: qunit task doesnt exist in gruntfile.

I didnt update dist yet, assuming grunt might do this?

@ericnewton76
Copy link
Author

ericnewton76 commented Apr 9, 2022

Note, I added a final commit which sets version to 3.9.0, bumping the 3.8 to 3.9 due to the new non-breaking functionality. I enscribed the version 3.9.0-645d8e1 in the files so that I could go ahead and integrate this into my project with the acknowledgement to somebody else that its a prerelease.

Also, I had to hand edit the dist/js/ files because the grunt replace task wasnt replacing //JG-CONTROLLER with the contents of dist/js/justifiedGallery.js. I tried a few things there and just couldnt get it to work and did it manually. (I'm running on Windows, perhaps something related to that)

I would recommend maybe retarget this PR into another branch or master and hard set the version to 3.9.0 (removing the prelease monikor) and do a final grunt run.

Copy link

@Imran-imtiaz48 Imran-imtiaz48 left a comment

Choose a reason for hiding this comment

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

Code Review

PR Title: thumbnailPath function can now signal to revert to default functionality

File Changes:

  1. dist/css/justifiedGallery.css
  2. dist/css/justifiedGallery.min.css
  3. dist/js/jquery.justifiedGallery.js

File: dist/css/justifiedGallery.css

Changes:

  • Updated version number from v3.8.1 to v3.9.0-645d8e1.
  • Changed copyright year from 2020 to 2022.
  • Updated project URL.

Review Comments:

  • Version Update: The version update reflects the latest changes, which is good practice.
  • Consistency: Ensure that the URL and copyright year are consistently updated across all documentation and files.
  • No Improvements Needed: These changes are straightforward and necessary for version tracking.

File: dist/css/justifiedGallery.min.css

Changes:

  • Same changes as in justifiedGallery.css.

Review Comments:

  • Consistency: Good job maintaining consistency across the minified CSS file as well.

File: dist/js/jquery.justifiedGallery.js

Changes:

  1. Header Update:

    • Updated version number, project URL, and copyright year.
  2. Function newSrc:

    • Added initialization of newImageSrc to null.
    • Added a check if (newImageSrc == null) to ensure newImageSrc is reassigned if thumbnailPath is not used.
  3. Error Handling in Image Load:

    • Changed the binding context from this to self by defining var self = this.
  4. Minor Formatting and Structural Changes:

    • Adjusted spacing and alignment for better readability.

Review Comments:

  1. Header Update:

    • Consistency: The updates to version, URL, and copyright are necessary and correct.
  2. Function newSrc:

    • Improvement: Initializing newImageSrc to null and adding a conditional check improves the robustness of the function by ensuring newImageSrc is always defined before being returned.
    • Suggestion: Instead of checking if (newImageSrc == null), consider using strict equality if (newImageSrc === null) for better type safety.
  3. Error Handling:

    • Improvement: Changing the context binding method from this to self improves readability and avoids potential issues with the this context in JavaScript.
    • Alternative Approach: Consider using an arrow function for more concise code:
      $image.one('error', () => {
          this.resetImgSrc($image); // revert to the original thumbnail
      });
    • Pros and Cons: The arrow function approach eliminates the need to define var self = this, but using self is also a clear and well-understood method.
  4. Formatting and Structure:

    • Readability: The adjustments to spacing and alignment make the code easier to read and maintain, which is beneficial for future developers working on this project.

Overall Summary:

The changes made in this commit enhance the robustness, readability, and maintainability of the code. The context binding improvement and initialization of variables are particularly beneficial. The version and metadata updates are also necessary for accurate version tracking. The alternative approaches suggested can be considered to further modernize the code.

Great work on these updates! If further testing reveals no issues.

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.

2 participants