-
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
Fix reuseIdentifier for empty message cells #203
Conversation
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.
Cool cool!
@@ -14,7 +14,7 @@ internal final class MessageThreadsDataSource: ValueCellDataSource { | |||
} | |||
|
|||
internal func emptyState(isVisible: Bool) { | |||
self.set(cellIdentifiers: isVisible ? ["MessageThreadsEmptyState"] : [], | |||
self.set(cellIdentifiers: isVisible ? ["MessageThreadEmptyStateCell"] : [], |
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.
Possible to write a test? Also would be nice to throw this in an enum or constant.
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 i could write a test. i hate loose strings and would love to have an enum for all of our cell names, but it seems like a separate task?
@@ -7,14 +7,16 @@ internal final class MessageThreadsDataSource: ValueCellDataSource { | |||
case messageThreads | |||
} | |||
|
|||
internal let emptyStateCellIdentifier = String(describing: MessageThreadEmptyStateCell.self) |
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.
hey @stephencelis in lieu of a more standardized way of accessing these loose cell names across the codebase, I opted for this constant. Let me know if this works.
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.
Sounds good! Wanna make it a static?
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.
will do!
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.
Thanks for the tests!
What
Was testing something for @tiegz and found a bug where we didn't have the correct reuse identifier for empty Messages cells! Woops. This fixes that as well as points us to
master
onReactiveExtensions
with @stephencelis's latest update.