observerevent event type issue fixed #2
Conversation
Looks like the MAKE_SYSTEM_PROP was not setting event values properly. I think keeping it in Module file is a good idea. Also, DATA_EVENT_TYPE_VALUE was set to FIRDataEventTypeChildAdded. Now changed to FIRDataEventTypeValue.
ios/Classes/FirebaseDatabaseModule.m
Outdated
| MAKE_SYSTEM_PROP(DATA_EVENT_TYPE_ADD, FIRDataEventTypeChildAdded); | ||
| MAKE_SYSTEM_PROP(DATA_EVENT_TYPE_REMOVE, FIRDataEventTypeChildRemoved); | ||
| MAKE_SYSTEM_PROP(DATA_EVENT_TYPE_MOVE, FIRDataEventTypeChildMoved); | ||
| MAKE_SYSTEM_PROP(DATA_EVENT_TYPE_CHANGE, FIRDataEventTypeChildChanged); |
There was a problem hiding this comment.
Looks good! One typo I noticed (that was in my initial implementation as well) is that we should include the CHILD phrase as well, e.g.:
DATA_EVENT_TYPE_CHILD_ADDEDDATA_EVENT_TYPE_CHILD_REMOVEDDATA_EVENT_TYPE_CHILD_MOVEDDATA_EVENT_TYPE_CHILD_CHANGED
Can you change that in code and docs?
Observerevent name changed. Documentation updated.
|
I have made the changes as suggested. Please check. Thanks! |
|
It should be a patch bump (1.1.1) instead of a minor bump (1.2.0), referring to the semver specs. Also, the constants should be suffixed with |
1. Typo fixed. 2. According to https://semver.org the patch version should be 1.1.1 Now it has been corrected.
|
Sorry, I should pay more attention to details. This is my first time contributing to other project. I have build the module with Titanium SDK 7.1.1 There are changes in titanium.xcconfig. Is that OK? Please find the attached zip file. |
|
Looks perfect! PR approved. Keep up the great contributions to the community. |
Looks like the MAKE_SYSTEM_PROP was not setting event values properly. I think keeping it in Module file is a good idea. Also, DATA_EVENT_TYPE_VALUE was set to FIRDataEventTypeChildAdded. Now changed to FIRDataEventTypeValue.