Skip to content

fix(active-index): resets index if activeIndex doesn't exist#9

Merged
andrewrubin merged 3 commits into
mainfrom
fix/active-index-reset
Sep 22, 2023
Merged

fix(active-index): resets index if activeIndex doesn't exist#9
andrewrubin merged 3 commits into
mainfrom
fix/active-index-reset

Conversation

@NicholasLancey
Copy link
Copy Markdown
Contributor

@NicholasLancey NicholasLancey commented Aug 29, 2023

Description

We discovered an edge case where the activeIndex did not exist. This often happens if the amount of GalleryItems changes based on the viewport width.

Solution

Adds a check to see if the activeIndex exists, if it doesn't run the goToIndex() function and default to 0.

Note

Fixes #3

Comment thread src/lib/components/gallery-main.jsx Outdated
} = useGallery()

useEffect(() => {
if (!galleryItems[activeIndex]) goToIndex(0)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we can lose the useEffect, and integrate this logic into the actual goToIndex function. That thing should be updated to be a bit more defensive anyway. Inside that function, maybe instead of just accepting that index is defined, let's first check that it's valid, and if not, set to zero.

I think with this useEffect solution, we'll still briefly get a warning, since side-effects run after the render phase.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Agree with this one, otherwise we'll always have to resort to this useEffect trick.

@NicholasLancey
Copy link
Copy Markdown
Contributor Author

A bit of back and fourth with @andrewrubin on this one.

  1. I've added a check to validate the index passed through to the goToIndex function. That's unrelated to the issue addressed in this PR.
  2. I've moved the useEffect to the context. Unfortunately I can't think of another way for this to work - as the function needs to run every time items changes.

@andrewrubin
Copy link
Copy Markdown
Member

Agreed, thanks for entertaining the idea though. Tested on my end and working. Here's a screencap where items changes at 768:

Screen.Recording.2023-09-21.at.5.44.18.PM.mp4

@andrewrubin andrewrubin self-requested a review September 22, 2023 00:46
andrewrubin
andrewrubin previously approved these changes Sep 22, 2023
@andrewrubin
Copy link
Copy Markdown
Member

Merging this one

@andrewrubin andrewrubin merged commit d1ed43e into main Sep 22, 2023
@andrewrubin andrewrubin deleted the fix/active-index-reset branch September 22, 2023 21:04
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.

Reset active gallery item if activeIndex is undefined

3 participants