-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add dictionary ios #4
base: master
Are you sure you want to change the base?
Conversation
arribbar
commented
Jul 27, 2017
- Add function setDictionary to iOS and rename set() by setString()
- Update docs
@alinz What do you think about implementing the bucket part into react-native-share-extension? |
@arribbar thanks for this PR. The thing is share-extension is not required to this package to run. because of that I kind of let the developer decide what library they want to use with share extension. |
Thanks for your quick reply. |
@alinz Do you mind to merge this PR, please ? :) |
@arribbar I have couple of name conventions which need to be resolve. I have already mentioned them in code review. |
lib/bucket/index.ios.js
Outdated
|
||
const Bucket = { | ||
set: (key, value, groupName) => { | ||
RNSKBucket.set(key, value, groupName) | ||
setString: (key, value, groupName) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the explicit name for it. It has to be simple enough. set, get and remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you use Flow type ? Do you have any link to help me use it ?
I tried something like :
set: (key: String, value: (String | Object), groupName: String) => {
console.log(' _______ type is ', typeof (value));
if (typeof (value) === 'string') {
RNSKBucket.setString(key, value, groupName);
} else if (typeof (value) === 'object') {
RNSKBucket.setDictionary(key, value, groupName);
}
},
But, in this way, you can give any type to value (that's why I added a else if)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ @alinz I'll need your help when you have some time :)
lib/bucket/index.android.js
Outdated
@@ -3,7 +3,7 @@ import { NativeModules } from 'react-native' | |||
const { RNSKBucket } = NativeModules | |||
|
|||
const Bucket = { | |||
set: (key, value, groupName) => { | |||
setString: (key, value, groupName) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of explicit about name, we can use flowtype to enforce the type
Can you take a look at the last changes @alinz, please ? |
# |-------- 50 characters for the title --------| # If applied, this commit will... # |----------------- 72 characters for the body ----------------------| # Why is this change needed? How do you solve it ?
# |-------- 50 characters for the title --------| # If applied, this commit will... # |----------------- 72 characters for the body ----------------------| # Why is this change needed? How do you solve it ?