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

Initialize galleryHeightToSet in constructor #374

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

drzony
Copy link

@drzony drzony commented Sep 25, 2022

When this variable is not initialized the height of the gallery is not set properly, so:

  • spinner is not visible on long loads
  • the rows do not appear one by one
  • the gallery is not visible until it's completed

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.

Review:

The changes seem to focus on:

  1. Updating Copyright Year:

    • Several files (.css, .js, and minified versions) have been updated to change the copyright year from 2020 to 2022.
  2. Initialization of galleryHeightToSet:

    • A new property galleryHeightToSet is initialized to 0 in the constructor of the JustifiedGallery class in JavaScript files.

Detailed Analysis:

  1. Copyright Year Update:

    • Positive: This update ensures that the files have the correct copyright year, which is essential for maintaining accurate legal documentation.
    • Negative: None.
  2. Initialization of galleryHeightToSet:

    • Positive:
      • Initializing galleryHeightToSet to 0 improves the robustness of the code by ensuring the property has a defined value when the object is created. This can help prevent potential issues related to uninitialized variables.
    • Negative:
      • There might be an assumption that galleryHeightToSet should always start at 0. This should be confirmed to avoid unexpected behavior in certain scenarios.

Improvement Suggestions:

  1. Documentation:

    • Ensure the new property galleryHeightToSet is documented within the code. Explain its purpose and how it should be used or modified.
  2. Unit Tests:

    • Add unit tests to verify that the galleryHeightToSet property behaves as expected. This includes checking its initial value and any subsequent changes during the object's lifecycle.
  3. Code Comments:

    • Add comments in the constructor to indicate why galleryHeightToSet is being initialized to 0. This will help future developers understand the reasoning behind this change.
  4. Code Consistency:

    • Ensure that the introduction of galleryHeightToSet is consistent with other properties in the constructor. If other properties follow a specific naming or initialization convention, galleryHeightToSet should align with these conventions.
  5. Comprehensive Update:

    • Review the entire codebase to ensure there are no other places where galleryHeightToSet might be used uninitialized. This could involve searching for references to this property and ensuring they all handle the initialization properly.

Here is an example of how the constructor with comments might look after improvements:

function JustifiedGallery($gallery, settings) {
    this.checkWidthIntervalId = null;
    this.galleryWidth = $gallery.width();
    this.$gallery = $gallery;
    
    // Initialize galleryHeightToSet to ensure it has a defined starting value
    this.galleryHeightToSet = 0;
}

Conclusion:

The provided changes are a positive step towards improving code robustness and maintaining up-to-date legal documentation. By adding documentation, unit tests, and comments, the changes can be further enhanced to ensure clarity and reliability.

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.

None yet

2 participants