Skip to content

Fix: Hide loading indicator if carousel loading fails.#11389

Merged
jimchamp merged 3 commits intointernetarchive:masterfrom
SAYAN02-DEV:fix-loading-indicator
Nov 4, 2025
Merged

Fix: Hide loading indicator if carousel loading fails.#11389
jimchamp merged 3 commits intointernetarchive:masterfrom
SAYAN02-DEV:fix-loading-indicator

Conversation

@SAYAN02-DEV
Copy link
Contributor

Closes #11380

fix: This feature hides the carousel loading indicator if carousel loading fails.

Technical

In openlibrary/openlibrary/plugins/openlibrary/js/lazy-carousel.js in catch block I added a condition to check if loadingIndicator is visible a then loadingIndicator.classList.add('hidden');
hides the loadingIndicator.

.catch(() => {
            if(loadingIndicator){
                loadingIndicator.classList.add('hidden');
            }
            const retryElem = target.querySelector('.lazy-carousel-retry')
            retryElem.classList.remove('hidden')
        })

Screenshot

before:
image
after:
unnamed

Copilot AI review requested due to automatic review settings October 29, 2025 17:32
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds defensive null-checking before hiding the loading indicator in the error handler of the lazy carousel functionality. This prevents potential runtime errors when the loading indicator element doesn't exist.

  • Added null check for loadingIndicator in the catch block before attempting to hide it

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 85 to 87
if (loadingIndicator){
loadingIndicator.classList.add('hidden');
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (loadingIndicator){
loadingIndicator.classList.add('hidden');
}
loadingIndicator.classList.add('hidden');

loadingIndicator will exist, and is accessed in the previous then call. No need for the if statement here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That is to say: This will always be true regardless of the element being hidden.

Copy link
Collaborator

@jimchamp jimchamp left a comment

Choose a reason for hiding this comment

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

Thanks @SAYAN02-DEV.

As things are today, your if condition will always be true. Please remove it.

Comment on lines 85 to 87
if (loadingIndicator){
loadingIndicator.classList.add('hidden');
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

That is to say: This will always be true regardless of the element being hidden.

@jimchamp jimchamp added the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Nov 4, 2025
Co-authored-by: jimchamp <28732543+jimchamp@users.noreply.github.com>
@github-actions github-actions bot removed the Needs: Submitter Input Waiting on input from the creator of the issue/pr [managed] label Nov 4, 2025
@SAYAN02-DEV
Copy link
Contributor Author

@jimchamp I got it and removed the if condition.

@jimchamp jimchamp merged commit f8f2b56 into internetarchive:master Nov 4, 2025
4 checks passed
@SAYAN02-DEV SAYAN02-DEV deleted the fix-loading-indicator branch November 18, 2025 06:08
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.

Loading indicator should be hidden when carousel fails to load

2 participants