Conversation
|
Any possibility to get this fix in @rishabhbhatia? without it the lib won't work for up-to-date RN projects. |
src/AwesomeAlert.js
Outdated
| export default class AwesomeAlert extends Component { | ||
| constructor(props) { | ||
| super(props); | ||
| this.backHandlerSubscription = null; // ✅ Değişkeni baştan tanımla |
There was a problem hiding this comment.
| this.backHandlerSubscription = null; // ✅ Değişkeni baştan tanımla | |
| this.backHandlerSubscription = null; |
| } = data; | ||
|
|
||
| return ( | ||
| <TouchableOpacity style={[styles.button, { backgroundColor }, buttonStyle]} testID={testID} onPress={onPress}> |
There was a problem hiding this comment.
this seems unnecessary, can we fix the linting here back to original
| }; | ||
|
|
||
| return ( | ||
| <View style={[styles.container, alertContainerStyle]}> |
There was a problem hiding this comment.
this seems unnecessary, can we fix the linting here back to original
| </Modal> | ||
| ) : this._renderAlert() | ||
| : null; | ||
| wrapInModal ? ( |
There was a problem hiding this comment.
same feedback for linting, keep the original
src/AwesomeAlert.js
Outdated
|
|
||
| componentWillUnmount() { | ||
| HwBackHandler.removeEventListener(HW_BACK_EVENT, this._handleHwBackEvent); | ||
| this.backHandlerSubscription?.remove(); // ✅ Eğer varsa kaldır |
There was a problem hiding this comment.
| this.backHandlerSubscription?.remove(); // ✅ Eğer varsa kaldır | |
| this.backHandlerSubscription?.remove(); |
| HwBackHandler.removeEventListener(HW_BACK_EVENT, this._handleHwBackEvent); | ||
| this.backHandlerSubscription?.remove(); // ✅ Eğer varsa kaldır | ||
| } | ||
|
|
There was a problem hiding this comment.
seems unnecessary, can we keep the original
|
I see a @foxriver76 for sure, happy to merge these and release a new version once the necessary changes are made on this PR. |
Applied requested changes: removed comments, reverted linting, fixed props.show init
|
Hey @rishabhbhatia, thanks for the feedback! ✅ I’ve made all the requested changes: Removed all the // comments for clarity. Reverted the unnecessary lint changes back to the original formatting. Set showSelf to initialize from props.show as suggested. Let me know if there’s anything else I should adjust. Thanks again for your time and for considering the PR! |
rishabhbhatia
left a comment
rishabhbhatia
left a comment
There was a problem hiding this comment.
thank you for making those changes. Same feedback as before, we'd want to remove all the extra code linting that's bloating the diff. One additional comment on an extra change made to showSelf.
If i had to guess, it might be your IDE auto-linting the file with your local settings once you finish making the changes. Look for the option to save without formatting. With the current change you would see the test fail too.
my expectation from this PR would be to consist a change for initialize the backhandler, subscribe to it when component mounts and remove the subscription on unmount.
| this.state = { | ||
| showSelf: false, | ||
| }; | ||
| this.state = { |
There was a problem hiding this comment.
this needs to be reverted back to how it was before
|
|
||
| return ( | ||
| <TouchableOpacity style={[styles.button, { backgroundColor }, buttonStyle]} testID={testID} onPress={onPress}> | ||
| <TouchableOpacity style={[styles.button, { backgroundColor }, buttonStyle]} testID={testID} onPress={onPress}> |
There was a problem hiding this comment.
there's still an extra space here that makes this change show up in the diff of the PR. If you remove that extra spacing, this would not show up here. Thats the state we'd want to be in.
| <TouchableOpacity style={[styles.button, { backgroundColor }, buttonStyle]} testID={testID} onPress={onPress}> | |
| <TouchableOpacity style={[styles.button, { backgroundColor }, buttonStyle]} testID={testID} onPress={onPress}> |
| </View> | ||
| </Animated.View> | ||
| </View> | ||
| <View style={[styles.container, alertContainerStyle]}> |
There was a problem hiding this comment.
same as above, the green square you're seeing is the extra unwanted spacing that we want to remove. This change should not show up in the PR diff.
3 participants
I made the necessary changes to fix crashes in my React Native project caused by the back button on Android devices and the closing of Awesome Alerts using BackHandler.