Size Change: 0 B

Total Size: 1.24 MB

_{compressed-size-action}

Added @geriux too in order to have a reviewer with more experience on React native (in case)

Thanks for the ping! I've tested this and there's an issue selecting a gradient color:

Since this PR changes the declaration to a variable, there's a line that's calling it before its declaration (see and this)

Since this PR changes the declaration to a variable, there's a line that's calling it before its declaration

Good catch! Weird that the linter didn't catch it though

Awesome catch @geriux, thank you! I agree with @ciampo, odd that this wasn't caught by the linter (though I should have seen it myself 😉 )

I've reordered things so that should hopefully not be an issue any longer!

@geriux would you be able to take another look? Thank you!

Thanks for updating the code! I've tested again and I found another issue with the useEffect, I wonder if we could remove isSelectedCustom as a dependency and skip this line from the exhaustive-deps check so it can unblock the rest of the work.

We can keep the isSelectedCustom changes using useCallback since that doesn't affect the functionality of the component.

This is the issue we have with the useEffect change, the scroll keeps going to the start of the list:

Thank you @geriux for taking another look!

I wonder if we could remove isSelectedCustom as a dependency and skip this line from the exhaustive-deps check so it can unblock the rest of the work.

This is usually a code smell, hinting that the code could be rewritten differently. I looked into the code a bit more, and noticed that the useEffect in question may also need to rely on scrollToEndWithDelay (weird that the linter isn't picking up on this, either).

If we have to disable eslint, I'd rather do it without introducing further runtime changes — that is, undo all changes from this PR, and simply add a comment to ignore eslint for the original error (and add a link to this PR as the explanation).

What do folks think?

This is usually a code smell, hinting that the code could be rewritten differently. I looked into the code a bit more, and noticed that the useEffect in question may also need to rely on scrollToEndWithDelay (weird that the linter isn't picking up on this, either).

Definitely, adding more things to the dependencies list might cause other side effects not expected when this component was created. And I agree that it would need to be refactored a so it works correctly.

If we have to disable eslint, I'd rather do it without introducing further runtime changes — that is, undo all changes from this PR, and simply add a comment to ignore eslint for the original error (and add a link to this PR as the explanation).

I like that suggestion, until we have some time to check that component and make the necessary code changes.

I'm actually happy we are planning to add the exhaustive-deps as it will add extra checks to prevent things like this.

Sounds like trying to get this one safely passing exhuastive-deps is triggering a larger domino effect than it makes sense to tackle right now, so I agree. I'll roll back this changes and replace them with an exclusion referencing this PR 👍

thank you @chad1008 for taking the time to work on this, even if we didn't "fix" the issue, this was definitely not wasted time!

+1 to that! Thank you!