-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Adds interface for localized push notifications #769
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
Conversation
|
@acinader can you have a look on this one? |
README.md
Outdated
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.
make dashboard a link. for bonus points make push notifications a link to doc too ;)
src/dashboard/Push/PushNew.react.js
Outdated
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.
.filter(locale => !(locale === '' || locale === undefined))
(since you kept the one in the dashboard ;))
src/dashboard/Push/PushNew.react.js
Outdated
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.
prefer const? or am I missing where it gets edited?
src/dashboard/Push/PushNew.react.js
Outdated
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.
a comment showing the string we're operating on here please.
src/lib/ParseApp.js
Outdated
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.
as written, this is exactly the same as just
return this.serverInfo.features.push.localization
it's like boolean day or something ;)
if this.serverInfo.features.push.localization might just be truthy instead of actual true then === wont work and you'd want return !!this.serverInfo.features.push.localization instead?
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.
you are right :)
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.
eb926ea to
768341d
Compare
- This is somewhat different from parse.com and probably more efficient
768341d to
422a009
Compare
|
@acinader adressed the nits + merge conflicts. |
| // Gather the translations, and inject into the payload | ||
| Object.keys(changes).forEach((key) => { | ||
| // translations are stored as `tranlation[lang]` strings as keys, | ||
| // this is why we slice it this way |
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.
Clear. thanks...just thinking out loud, not asking for change....
const needle = 'translation[';
if (key.indexOf(needle) === 0) {
const locale = key.slice(needle.length, key.length - 1);
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.
Eheh, if you need a comment, many others may need a comment. it's better with it than without.
acinader
left a comment
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.
lgtm (assuming tests pass of course.)
|
Looks like I broke master by merging #709 |
* Adds interface for localized push notifications - This is somewhat different from parse.com and probably more efficient * Fix lint issues * nits * nit * fixes compile issues * cache node_modules
See also: parse-community/parse-server#4130