-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[MBL-1036] Block User Popup + Banner Message #1880
Conversation
…e project page after blocking
…bleviewcontroller * this only needs to be forced in messages and comments
Codecov Report
@@ Coverage Diff @@
## main #1880 +/- ##
==========================================
- Coverage 83.83% 83.77% -0.07%
==========================================
Files 1224 1224
Lines 111349 111457 +108
Branches 29601 29636 +35
==========================================
+ Hits 93349 93368 +19
- Misses 16989 17073 +84
- Partials 1011 1016 +5
... and 1 file with indirect coverage changes 📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
1d303f8
to
824ab38
Compare
Kickstarter-iOS/Features/MessageBanner/Controller/MessageBannerViewController.swift
Show resolved
Hide resolved
Kickstarter-iOS/Features/Messages/Controller/MessagesViewController.swift
Outdated
Show resolved
Hide resolved
Kickstarter-iOS/Features/Comments/Controllers/CommentRepliesViewController.swift
Outdated
Show resolved
Hide resolved
…e the banner stretches to the width of its parent view
extension CommentRepliesViewController: MessageBannerViewControllerDelegate { | ||
func messageBannerViewDidHide(type _: MessageBannerType) { | ||
self.commentComposer.isHidden = false | ||
self.view.isUserInteractionEnabled = true |
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.
I took a peek in MessageBannerViewController
, and it does already reference self.view.superview
, so it might be safe to move isUserInteractionEnabled
in there. But that's not a blocker, as previously discussed.
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.
I didn't test on my machine, but 👀 code LGTM.
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.
Looks good! I added some nits that would be nice to fix before merging, but I don't think any of them are complicated enough that they'll require another review pass.
.showBanner(with: .success, message: "This user has been successfully blocked") | ||
} else { | ||
self?.messageBannerViewController? | ||
.showBanner(with: .error, message: "Your request did not go through. Try again.") |
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.
Nit: Add a TODO here and for the other banner messages to update them once translated strings are ready
@@ -170,16 +189,26 @@ extension MessagesViewController: BackingCellDelegate { | |||
// MARK: - MessageCellDelegate | |||
|
|||
extension MessagesViewController: MessageCellDelegate { | |||
func messageCellDidTapHeader(_ cell: MessageCell, _: User) { | |||
func messageCellDidTapHeader(_ cell: MessageCell, _ author: User) { |
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.
Nit: I don't think author
is the right name in this context - just user
would be better
@@ -99,6 +108,7 @@ public final class MessageBannerViewController: UIViewController, NibLoading { | |||
|
|||
if !isHidden { | |||
self.view.superview?.bringSubviewToFront(self.view) | |||
self.view.superview?.isUserInteractionEnabled = false |
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.
Did you want the message banner class to be responsible for turning on and off user interaction? I'm okay with that (as long as that works for all current call sites), but then the corresponding lines of code in the individual classes need to be deleted.
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.
Yep - It does work for the other call sites. Good catch. I thought I had gotten them all. Will remove it from the last 2 classes.
📲 What
🤔 Why
Rather than sending a blocking request from the action sheet, we're going to use this confirmation alert pattern so that we can provide extra information and double-check that they want to block a user.
🛠 How
The key areas are:
I'm taking advantage of the existing MessageBannerViewController that we use elsewhere in the app to display API response success and error banners. This means that each ViewController, except ProjectPage, needs to conform to
MessageBannerViewControllerPresenting
and present an instance of the banner when the corresponding ViewModel outputs a response from the API (which isn't wired up yet).One caveat is that to keep users from spamming the block action sheet and alert, I'm disabling user interaction on the parent view until the success/error banner message is finished displaying. Once the success banner is dismissed on the project page, we auto-dismiss the view since the user doesn't want to see that content anymore. This could also set us up to more easily refresh our app content.
So basically I've:
userBlocked
output notifies the ViewController of which banner to display. Success or error.👀 See
✅ Acceptance criteria
Run the AppCenter Beta build. Make sure you're logged in and that the Block User FF is on.