-
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
[NT-534] Show User Name and Avatar on the Manage Pledge screen #938
Conversation
@@ -10,7 +10,7 @@ private enum Layout { | |||
} | |||
} | |||
|
|||
protocol ProjectPamphletCreatorHeaderCellDelegate: class { | |||
protocol ProjectPamphletCreatorHeaderCellDelegate: AnyObject { |
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.
This was done by swiftformat
, seems it was missed in a recent PR.
@@ -105,6 +105,11 @@ public let checkoutBackgroundStyle: ViewStyle = { (view: UIView) in | |||
|> \.backgroundColor .~ UIColor.ksr_grey_300 | |||
} | |||
|
|||
public let checkoutLabelStyle: LabelStyle = { label in |
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.
To prevent color blending on labels.
…NT-534-user-name-and-avatar
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.
Excellent work! Works great, I just had one or two minor questions.
@@ -19,12 +19,17 @@ final class ManagePledgeViewControllerTests: TestCase { | |||
super.tearDown() | |||
} | |||
|
|||
func testView() { | |||
func testView_CurrentUser_IsBacker() { |
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.
Is this just future-proofing? As I understand it from our discussions the user will always be the backer? Which is why I removed the creator strings in #934 🤔
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.
Yeah sort of - although there's currently no way to get to this state, the VM does support it so I created a screenshot so we can verify that it visually matches what we expect.
.map(\.name) | ||
|
||
self.backerImageURLAndPlaceholderImageName = userBackingProject | ||
.map(\.avatar.small) |
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.
😮 🤯
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.
Looking good, @ifbarrera . I left a few suggestions and comments, but nothing serious.
Kickstarter-iOS/Views/Controllers/ManagePledgeSummaryViewController.swift
Outdated
Show resolved
Hide resolved
self.viewModel.outputs.backerImageURLAndPlaceholderImageName | ||
.observeForUI() | ||
.on(event: { [weak self] _ in | ||
self?.circleAvatarImageView.af_cancelImageRequest() |
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 wonder if we need to cancel the image request (and set image to nil
) here. I this this is usually done on Cells, where the imageViews will be recycled due scrolling.
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.
Hmm is it possible that the user could navigate away from the VC before the request has completed, and then navigates back to it? I figure it doesn't hurt 🤔
@@ -41,11 +46,42 @@ final class ManagePledgeViewControllerTests: TestCase { | |||
} | |||
} | |||
|
|||
func testView_CurrentUser_IsNotBacker() { |
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 think the name can cause confusion in the future, since only creators and backers will have access to this screen (and for now, we're not showing this screen to creators). So I think we could either delete it or maybe rename and update it to test view when user is creator of project?
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 think it's helpful to have this screenshot as a reference for how the view should look if the current user is not the backer 🤷♀
📲 What
This PR adds the user's avatar and name on the manage pledge screen if the current user is the backer of that project.
Also adds a placeholder image for remote avatar URL loading so the transition is more subtle.
🤔 Why
More visual context on the manage pledge screen.
🛠 How
We check whether the current user is the backer of the project, and if they are, we show their avatar and name on the Manage/View pledge screen. If the current user is not the backer of the project, we hide the avatar and name labels.
👀 See
Trello, screenshots, external resources?
♿️ Accessibility
🏎 Performance
✅ Acceptance criteria