Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Toggling following in settings #259

Merged
merged 3 commits into from
Jun 7, 2018
Merged

Toggling following in settings #259

merged 3 commits into from
Jun 7, 2018

Conversation

eoji
Copy link
Contributor

@eoji eoji commented Jun 7, 2018

what

Users should be able to toggle whether or not they can be following or social via settings. This change also updates whether the Backed by people you follow section is visible in the drawer.
This is a destructive change so we need to confirm they definitely want to do this.

why

G
D
P
R

how 馃

The optIntoFollowing input determines whether user has checked the switch on or off. The optOutOfFollowing input determines whether user has confirmed turning Following off.

When optIntoFollowing is true, we just turn Following on.
When optIntoFollowing is false, we show the confirmation dialog. This dialog is not cancelable, I expect demand, they make a choice!

When optOutOfFollowing is true, that's a confirmation, we turn Following off.
When optOutOfFollowing is false, we change the switch back to on!

see 馃憖

First, I'm social and my drawer shows Backed by people you follow. I attempt to turn it off, cancel. I attempt to turn it off again, and confirm. My drawer no longer shows Backed by people you follow.
2018-06-07 13_37_43

@eoji eoji requested a review from ifbarrera June 7, 2018 17:41

this.optOutOfFollowing
.compose(bindToLifecycle())
.filter(optOut -> !optOut)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it might make sense to consolidate these two subscribers into one:

.subscribe(optOut -> 
   if (optOut) {
     this.userInput.onNext(this.userOutput.getValue().toBuilder().social(false).build())
   } else {
     this.hideConfirmFollowingOptOutPrompt.onNext(null)
   })

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm glad you called this out because I had it that way and changed it. What do you think is best? I was trying to avoid doing an if/else because it's not very functional 馃槶but then this way is not very readable.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I thought there might have been a reason you went this way. I'm ok with it, just wanted to see the thinking behind it! Thanks! 猸愶笍

Copy link

@ifbarrera ifbarrera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! I just had the one suggestion, not a blocker though. Nice work! 馃帀

@eoji
Copy link
Contributor Author

eoji commented Jun 7, 2018

Following consent UI

@eoji
Copy link
Contributor Author

eoji commented Jun 7, 2018

@eoji eoji merged commit e857094 into master Jun 7, 2018
@eoji eoji deleted the following branch June 7, 2018 18:51
Rcureton pushed a commit that referenced this pull request Jul 31, 2018
eoji added a commit that referenced this pull request Feb 4, 2019
* Add CloudFormation for CircleCI user

* Fix indentation on CloudFormation template

* Add GetObject to CI user
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants