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 online notification switch to settings #828

Merged
merged 1 commit into from Jul 9, 2017

Conversation

Projects
None yet
4 participants
@kunall17
Contributor

kunall17 commented Jul 8, 2017

No description provided.

@smarx

This comment has been minimized.

smarx commented Jul 8, 2017

Automated message from Dropbox CLA bot

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

@borisyankov

Nice. Relatively minor changes needed.

auth,
'settings/notifications',
res => res,
JSON.parse(JSON.stringify({ // To remove undefined values

This comment has been minimized.

@borisyankov

borisyankov Jul 8, 2017

Contributor

Extract that to an utility function that is called removeUndefinedValues then you don't need a comment.

@@ -8,7 +8,7 @@ export default class ZulipSwitch extends React.PureComponent {
props: {
value?: boolean,
defaultValue: boolean,
onValueChange: (arg: boolean) => void,
onValueChange: (arg: boolean) => void | Promise<void>,

This comment has been minimized.

@borisyankov

borisyankov Jul 8, 2017

Contributor

This might be better not to change...

@@ -33,7 +33,8 @@ class SettingsScreen extends React.Component {
props: {
auth: Auth,
notifications: boolean,
offlinePushNotification: boolean,

This comment has been minimized.

@borisyankov

borisyankov Jul 8, 2017

Contributor

Don't over describe the name. We don't need the 'push' part in the name.

toggleOfflinePushNotifications(this.props.auth, value);
this.props.settingsChange('notifications', value);
handleOfflineNotificationChange = async (value) => {
await toggleMobilePushSettings({ auth: this.props.auth, offline: value });

This comment has been minimized.

@borisyankov

borisyankov Jul 8, 2017

Contributor

I don't think we need the await. This will make the toggle buttons look laggy.

@zulipbot

This comment has been minimized.

Member

zulipbot commented Jul 8, 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.

@zulipbot zulipbot added needs review and removed reviewed labels Jul 9, 2017

@kunall17

This comment has been minimized.

Contributor

kunall17 commented Jul 9, 2017

Done 👍

@borisyankov

This comment has been minimized.

Contributor

borisyankov commented Jul 9, 2017

Cool. Looks done.

@borisyankov borisyankov merged commit a7df8d7 into zulip:master Jul 9, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.05%) to 58.419%
Details

@zulipbot zulipbot removed the needs review label Jul 9, 2017

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