react-native-actionsheet icon indicating copy to clipboard operation
react-native-actionsheet copied to clipboard

Published 20 hours ago • verified publisher icon beefe

Update ActionSheetCustom.js

Open falcon027 opened this issue 4 years ago • 14 comments

fixes warning

falcon027 avatar May 07 '20 07:05 falcon027

Mb merge with small update?

klimdead avatar May 18 '20 21:05 klimdead

When will this get merged? The warning is very annoying...

bellini666 avatar May 27 '20 03:05 bellini666

Maybe @xiaoyann can merge?

pke avatar Jun 03 '20 15:06 pke

I have read the code. In my opinion, there is a better way to fix this warning. The method componentWillReceiveProps only re-calculate the value of this.translateY. To fix this warning, just move the calculation into shouldComponentUpdate like below:

Screen Shot 2020-06-10 at 11 05 41

It works fine because shouldComponentUpdate also be executed before re-rendering like componentWillReceiveProps. For the first rendering, translateY has been calculated in constructor.

There is no need to use UNSFAFE_componentWillReceiveProps anymore. It is unsafe :D

thanhtunguet avatar Jun 10 '20 04:06 thanhtunguet

I think shouldComponentUpdate is not supposed to modify state? It's basically a read-only function that should have no side effects. I think maybe a better way would be to get rid of classes, bump the dependency on react version with hooks and use hooks. Update this packages major version and it would be a clean slate again.

pke avatar Jun 10 '20 06:06 pke

I think shouldComponentUpdate is not supposed to modify state? It's basically a read-only function that should have no side effects. I think maybe a better way would be to get rid of classes, bump the dependency on react version with hooks and use hooks. Update this packages major version and it would be a clean slate again.

Thank you for your comment. I will try to rewrite the component using Hook API.

thanhtunguet avatar Jun 10 '20 06:06 thanhtunguet

Thanks for your efforts, lets hope the repo owners still maintain this repo and merge some PRs.

pke avatar Jun 10 '20 07:06 pke

@thanhtunguet any updates on this?

pke avatar Jul 01 '20 10:07 pke

@thanhtunguet any updates on this?

I have rewritten the package and created a new issue #114 . It works but the stylesheet is broken. I don't have enough time to find out why.

thanhtunguet avatar Jul 01 '20 10:07 thanhtunguet

@thanhtunguet any updates on this?

See my package here:

https://www.npmjs.com/package/thanhtunguet-react-native-actionsheet I fixed styles and all warnings.

thanhtunguet avatar Jul 07 '20 22:07 thanhtunguet

Did you update your #114 accordingly?

pke avatar Jul 08 '20 11:07 pke

#114

Nope, I have to use this package in my project so that I can not wait for pull request acceptance or any reply of the issue. I have published the package with my own name.

Did you update your #114 accordingly?

Nope, I have closed the issue because of days I had not received any reply, published my own package to use in my project. I need someone to test it, tell me that it works properly so I will create a pull request back to this repository.

thanhtunguet avatar Jul 10 '20 16:07 thanhtunguet

@pke

See my #115 pull request please.

thanhtunguet avatar Jul 11 '20 08:07 thanhtunguet

Will this be merged any time soon ?

FadiAboMsalam avatar Oct 31 '20 16:10 FadiAboMsalam