Skip to content
This repository has been archived by the owner on Feb 27, 2023. It is now read-only.

Fix photos not updated when mediaList prop changes #86

Merged
merged 2 commits into from
Dec 27, 2018

Conversation

riwu
Copy link
Contributor

@riwu riwu commented Dec 27, 2018

Since props.mediaList is internalised as state in the root component, the library will ignore changes in props.mediaList, which will break use cases such as adding/deleting photos.

Solution: Move the selected state from the root component to the Photo component.

Also avoid using index as the key for the FlatList as it breaks the use cases mentioned above.

@halilb
Copy link
Owner

halilb commented Dec 27, 2018

This is a great fix. Thank you @riwu! And the PR looks clean. I should just make sure this doesn't break any corner cases.

And I have a favor to ask about this. Could you add a showcase about updated mediaList in the example project?

@riwu
Copy link
Contributor Author

riwu commented Dec 27, 2018

Added deletion to the grid example. Deletion doesn't work very intuitively for full screen mode though (user has to navigate to next photo after deleting), might want to switch FlatList for something simpler for the FullScreenContainer component.

@jothikannan
Copy link

@halilb this would be great if we have the delete in full screen too. I hope you will make it work.

@halilb halilb self-requested a review December 27, 2018 18:07
@halilb halilb merged commit 80ef351 into halilb:master Dec 27, 2018
@halilb
Copy link
Owner

halilb commented Dec 27, 2018

It's working great for the grid view @riwu. Thanks again!

It has some problems in full screen mode as you said. So I merged this PR and #87 to track that down. I hope I can find some free time to investigate that.

halilb added a commit that referenced this pull request Dec 28, 2018
Fix photos not updated when mediaList prop changes
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants