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(Picker): Watch for skin property changes #294

Merged
merged 1 commit into from
Jan 27, 2024

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Jan 22, 2024

Currently changes of properties, especially the skin prop, are not reactive.
Meaning you have to close and reload the picker if you change the prop.

So this watches on the prop so that you can modify it and the picker will react (e.g. we want to have the skin tone selector next to the search). Which works and looks like this for us:

vokoscreenNG-2024-01-22_01-32-48.mp4

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@serebrov
Copy link
Owner

@susnux Thanks for the PR!

Could you explain the problem a bit more? Checking in the demo app, the skin tone changes applied instantly:

Screen.Recording.2024-01-27.at.13.17.22.mov

The skin selector is in the separate component, Preview and it calls the onSkinChange() that also updates the activeSkin property. So I feel like the actual problem is not that properties are not reactive, but that skin goes out of sync with activeSkin.

Either way, the solution to watch for skin change and call onSkinChange automatically seems appropriate and I will merge the PR and release a new version.

@serebrov serebrov merged commit 6cdf347 into serebrov:master Jan 27, 2024
2 checks passed
@serebrov
Copy link
Owner

I released version 15.0.1 with this fix.

@susnux susnux deleted the fix/skin-tones-not-reactive branch January 28, 2024 16:51
@susnux
Copy link
Contributor Author

susnux commented Jan 28, 2024

Could you explain the problem a bit more? Checking in the demo app, the skin tone changes applied instantly:

Because in the default picker the skin selector is provided the onSkinChange function directly which then reacts to the skin change.

But if you have an external skin change by prop then it is not reactive (before this change here).

I released version 15.0.1 with this fix.

Thank you very much 🚀

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.

2 participants