-
Notifications
You must be signed in to change notification settings - Fork 0
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
Adds survey availability notices, optimizes survey list and triggers, fixes bug with Realm data #99
Conversation
@gbersani ping |
Also, this is likely going to need updated to work on Android. So I'd prefer this to happen after #100 |
Sounds good. |
# Conflicts: # android/app/src/main/AndroidManifest.xml # android/app/src/main/java/com/goodquestion/MainActivity.java # android/app/src/main/res/values/strings.xml # index.android.js # js/router/SharedNavigator.js # package.json
formId: 'string', | ||
surveyId: 'string', | ||
formId: {type: 'string', default: 'none'}, | ||
surveyId: {type: 'string', default: 'none'}, |
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.
Are we checking for 'none' somewhere that we are defaulting to this value? Perhaps a preference thing, but why not null?
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.
The reason I don't like defaulting to a 'none' string, is that its a truthy value and could lead to a potential bug.
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.
From what I found Realm can perform automatic migrations, having the default set can prevent a few crashes when updating objects when calling these new properties.
There's no easy way to check if a value is null with a query, as it forces strict type casting.
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.
Seems like its been added: realm 162.
This PR is a series of related tasks related to the survey list and triggers.
Realm version had to be updated to fix a common bug. Be sure to run
npm install
after merging this branch.Stories:
https://www.pivotaltracker.com/story/show/120683843
https://www.pivotaltracker.com/story/show/121071009
https://www.pivotaltracker.com/story/show/121131787
Features:
Changes:
Bug fixes: