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

fix: avoids remounting #13567

Conversation

dzucconi
Copy link
Member

@dzucconi dzucconi commented Mar 4, 2024

Closes DIA-456

Re: #13214 — this bug needs to be fixed rather than papered over by constantly remounting the filter whenever the route changes.

It's actually pretty concerning that one can't drive the filters with a simple route change. One would think that would be the primary way we'd change filter state...

Copy link
Contributor

@olerichter00 olerichter00 left a comment

Choose a reason for hiding this comment

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

Do you have an idea on how to update the filter state upon a simple route change? If I recall correctly, the issue is that the ArtworkFilterContextProvider uses the props to initialize the filter store but then does not update the store when the props change.

@dzucconi
Copy link
Member Author

dzucconi commented Mar 4, 2024

@olerichter00 I've seriously considered just rewriting the filters component to avoid having to fix anything in it 😆 — I'm not sure. If we can just update the internal state whenever the route changes, using an effect, that'd be great though there might be some complications with a recursive effect loop.

Comment on lines -70 to -71
// Setting the key here to enforce a remount of the component when changing pages or query parameter filters within the query.
key={match.location.pathname + match.location.search}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if I understand correctly why this causes the flashing images. Is it because forcing the ArtworkFilterContextProvider with the key prop also remounts it's children?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, key will forcibly re-render the component and anything under it. That means images will be re-loaded.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, then adding a key here is indeed not a good idea.

Copy link
Member

Choose a reason for hiding this comment

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

Ah @dzucconi - this was the same issue that fixed things before, I (think).

Re URL state -- yep, driving by URL state, we've been talking about this for a while. It seems straight forward 🤷

@olerichter00 olerichter00 merged commit 83e5cf4 into main Mar 4, 2024
11 checks passed
@olerichter00 olerichter00 deleted the DIA-456-clicking-an-item-in-the-artwork-grid-leads-to-a-rerender-of-all-images-and-blurhash branch March 4, 2024 15:45
@artsy-peril artsy-peril bot mentioned this pull request Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants