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 a setting for enabling offline push notifications #748

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@kunall17
Contributor

kunall17 commented Jun 26, 2017

This and the previous PR does not have the event updates, right now they are only being fetched on the first registration, what do you think do we require event updates for these things?

And also they have a lot unused json settings, which is not avoidable at this time as the server does not supports!

@smarx

This comment has been minimized.

smarx commented Jun 26, 2017

Automated message from Dropbox CLA bot

@kunall17, it looks like you've already signed the Dropbox CLA. Thanks!

@zulipbot

This comment has been minimized.

Member

zulipbot commented Jun 26, 2017

Heads up @kunall17, we just merged some commits (latest: f6513c6) that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Jun 26, 2017

Rebased

@timabbott

This comment has been minimized.

Member

timabbott commented Jun 26, 2017

We definitely want event updates for changes to this sort of setting.

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Jun 27, 2017

@timabbott Ok, will do in a separate PR!

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Jun 27, 2017

Merging your other PR conflicted with this one. Please rebase.

@zulipbot

This comment has been minimized.

Member

zulipbot commented Jun 27, 2017

Heads up @kunall17, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Jun 28, 2017

Merged in #754

@kunall17 kunall17 closed this Jun 28, 2017

@zulipbot zulipbot removed the needs review label Jun 28, 2017

@kunall17 kunall17 deleted the kunall17:patchnotification branch Jun 28, 2017

@timabbott

This comment has been minimized.

Member

timabbott commented Jul 7, 2017

Just took a quick look at this; we might also want to add the "send push notifications even when online" setting to this settings UI as well, since that's the other one that is very specifically about mobile.

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Jul 8, 2017

True. We might want to have the settings on two levels - 'Notifications' will navigate to another screen with the two options, 'Language' - will navigate to another screen with all language options.

@saketkumar

This comment has been minimized.

Member

saketkumar commented Jul 8, 2017

Can i take this up?

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Jul 8, 2017

The online setting is added in #828
Lets merge that PR as it has the reducers and other change, will do the two levels in another PR

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Jul 8, 2017

The two level thing is not an immediately required feature.
Also, I resurrected the React Navigation PR and it will be ready to merge soon, we better not add too many screens before that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment