-
Notifications
You must be signed in to change notification settings - Fork 992
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
facepile ( •_•) ( •_•)>⌐■-■ (⌐■_■) #181
Conversation
…evice and ", " in tests....
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.
lgtm!
this.friendAvatarUrl2.assertValues(project.friends().get(1).avatar().small()); | ||
this.friendAvatarUrl3.assertValues(project.friends().get(2).avatar().small()); | ||
this.friendAvatar2IsHidden.assertValue(false); | ||
this.friendAvatar3IsHidden.assertValue(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.
should we have a test for _withNoFriends
? :foreveralone:
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 testEmitsFriendBackingViewIsHidden
test handles that case! 🕵️♀️
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.
oh my bad ok!
|
||
return ksString.format("discovery_baseball_card_social_friends_are_backers", friends.size(), | ||
return ksString.format("discovery_baseball_card_social_friends_are_backers", friends.size() == 3 ? friends.size() - 1 : friends.size(), |
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 this is all a bit gnarly but it makes sense! If I have any refactoring ideas I'll chime 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.
looks great! just a few lil comments
final String friendName = friends.size() >= 1 ? friends.get(0).name() : ""; | ||
final String secondFriendName = friends.size() >= 2 ? friends.get(1).name() : ""; | ||
final String remainingCount = NumberUtils.format(Math.max(0, friends.size() - 2)); | ||
public static @NonNull String projectCardFriendNamepile(final Context context, final @NonNull List<User> friends, final @NonNull KSString ksString) { |
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.
@NonNull
Context
final String friendName; | ||
if (friends.size() < 3) { | ||
friendName = friends.size() >= 1 ? friends.get(0).name() : ""; | ||
} else { |
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 logic makes sense, but is a little hard to read through. maybe it'd be helpful to leave a lil comment here with an example of what friendName
would be set here
@@ -118,10 +120,30 @@ public ProjectCardViewHolder(final @NonNull View view, final @NonNull Delegate d | |||
.compose(observeForUI()) | |||
.subscribe(ViewUtils.setGone(this.featuredViewGroup)); | |||
|
|||
this.viewModel.outputs.friendAvatarUrl() | |||
this.viewModel.outputs.friendAvatar2IsHidden() |
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.
can rename these outputs from IsHidden
to IsGone
to more properly reflect what's happening in the view!
@@ -282,10 +304,10 @@ private void setDeadlineCountdownText(final @NonNull Project project) { | |||
this.deadlineCountdownUnitTextView.setText(ProjectUtils.deadlineCountdownDetail(project, context(), this.ksString)); | |||
} | |||
|
|||
private void setFriendAvatarUrl(final @NonNull String avatarUrl) { | |||
private void setFriendAvatarUrl(final @NonNull String avatarUrl, final @NonNull ImageView imageView) { |
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.
nice nice
android:layout_gravity="center_vertical" | ||
android:layout_width="wrap_content" | ||
android:layout_height="wrap_content" | ||
tools:text="Dancing Hot Dog, Frank, and 1 more are backers." /> |
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.
🌭
lgtm! |
👍 comments are helpful! |
* Facepile and new namepile logic with tests
what
Implementing ✨ facepile ✨ designs showing up to 3 friends' names and avatars.
SocialUtils
got hairy. Open to any and all suggestions.who is she