Skip to content

Commit e970f8f

Browse files
committed
Fix review findings in the date subheaders
- Key each date header by its label instead of the group's first comment id, so a header stays stable (no remove+add re-animation) when a newer comment is prepended into an existing date group. Labels are unique per contiguous group given the date sort. Drops the now-unneeded keyId field. - Uppercase the header label with the default locale, matching the legacy subheader's locale-aware textAllCaps (Kotlin's no-arg uppercase() is Locale.ROOT and mis-cases e.g. Turkish month names).
1 parent 019cb1c commit e970f8f

4 files changed

Lines changed: 24 additions & 18 deletions

File tree

WordPress/src/main/java/org/wordpress/android/ui/commentsrs/CommentsRsListRow.kt

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,7 @@ package org.wordpress.android.ui.commentsrs
22

33
/** A rendered row in the rs comment list: either a date-group header or a comment. */
44
sealed interface CommentsRsListRow {
5-
/** A date subheader. [keyId] is the first comment id in the group, for a stable LazyColumn key. */
6-
data class DateHeader(val label: String, val keyId: Long) : CommentsRsListRow
5+
data class DateHeader(val label: String) : CommentsRsListRow
76
data class Item(val comment: CommentRsUiModel) : CommentsRsListRow
87
}
98

@@ -12,13 +11,17 @@ sealed interface CommentsRsListRow {
1211
* list: a header before the first comment and before every comment whose date label differs from
1312
* the previous one. The label is the row's own [CommentRsUiModel.relativeDate] — the same
1413
* javaDateToTimeSpan value the legacy list groups by — so no extra date handling is needed here.
14+
*
15+
* The label doubles as the header's LazyColumn key: comments are date-sorted, so a label maps to
16+
* exactly one contiguous group (unique key), and keying by the label rather than the group's first
17+
* comment keeps the header stable when a newer comment is prepended into an existing group.
1518
*/
1619
fun withDateHeaders(comments: List<CommentRsUiModel>): List<CommentsRsListRow> {
1720
val rows = ArrayList<CommentsRsListRow>(comments.size + 1)
1821
var lastLabel: String? = null
1922
for (comment in comments) {
2023
if (comment.relativeDate != lastLabel) {
21-
rows.add(CommentsRsListRow.DateHeader(comment.relativeDate, comment.remoteCommentId))
24+
rows.add(CommentsRsListRow.DateHeader(comment.relativeDate))
2225
lastLabel = comment.relativeDate
2326
}
2427
rows.add(CommentsRsListRow.Item(comment))

WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsListItem.kt

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ import androidx.compose.ui.unit.dp
3939
import coil.compose.AsyncImage
4040
import org.wordpress.android.R
4141
import org.wordpress.android.ui.commentsrs.CommentRsUiModel
42+
import java.util.Locale
4243

4344
private val AVATAR_SIZE = 40.dp
4445
private val PENDING_INDICATOR_WIDTH = 4.dp
@@ -178,7 +179,9 @@ private fun AnnotatedString.Builder.boldRange(formatted: String, part: String) {
178179
@Composable
179180
fun CommentsRsDateHeader(label: String, modifier: Modifier = Modifier) {
180181
Text(
181-
text = label.uppercase(),
182+
// Locale-aware uppercase, like the legacy subheader's android:textAllCaps (Kotlin's no-arg
183+
// uppercase() is Locale.ROOT and would mis-case e.g. Turkish month names).
184+
text = label.uppercase(Locale.getDefault()),
182185
style = MaterialTheme.typography.labelMedium,
183186
color = MaterialTheme.colorScheme.onSurfaceVariant,
184187
maxLines = 1,

WordPress/src/main/java/org/wordpress/android/ui/commentsrs/screens/CommentsRsTabListScreen.kt

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ private fun CommentListContent(
148148
items = rows,
149149
key = { row ->
150150
when (row) {
151-
is CommentsRsListRow.DateHeader -> "header_${row.keyId}"
151+
is CommentsRsListRow.DateHeader -> "header_${row.label}"
152152
is CommentsRsListRow.Item -> row.comment.remoteCommentId
153153
}
154154
},

WordPress/src/test/java/org/wordpress/android/ui/commentsrs/CommentsRsListRowTest.kt

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ class CommentsRsListRowTest {
1717
val comment = comment(id = 1, date = "Today")
1818

1919
assertThat(withDateHeaders(listOf(comment))).containsExactly(
20-
DateHeader("Today", keyId = 1),
20+
DateHeader("Today"),
2121
Item(comment)
2222
)
2323
}
@@ -28,7 +28,7 @@ class CommentsRsListRowTest {
2828
val b = comment(id = 2, date = "Today")
2929

3030
assertThat(withDateHeaders(listOf(a, b))).containsExactly(
31-
DateHeader("Today", keyId = 1),
31+
DateHeader("Today"),
3232
Item(a),
3333
Item(b)
3434
)
@@ -42,26 +42,26 @@ class CommentsRsListRowTest {
4242
val d = comment(id = 4, date = "January 8")
4343

4444
assertThat(withDateHeaders(listOf(a, b, c, d))).containsExactly(
45-
DateHeader("Today", keyId = 1),
45+
DateHeader("Today"),
4646
Item(a),
4747
Item(b),
48-
DateHeader("Yesterday", keyId = 3),
48+
DateHeader("Yesterday"),
4949
Item(c),
50-
DateHeader("January 8", keyId = 4),
50+
DateHeader("January 8"),
5151
Item(d)
5252
)
5353
}
5454

5555
@Test
56-
fun `each header is keyed by the first comment in its group`() {
57-
val rows = withDateHeaders(
58-
listOf(
59-
comment(id = 10, date = "Today"),
60-
comment(id = 11, date = "Yesterday")
61-
)
62-
)
56+
fun `a header stays identical when a newer comment is prepended into its group`() {
57+
// The header is keyed by its label, so adding a same-day comment at the top of the group
58+
// must not change the header's identity (which would make it re-animate on refresh).
59+
val before = withDateHeaders(listOf(comment(id = 1, date = "Today")))
60+
val after = withDateHeaders(listOf(comment(id = 2, date = "Today"), comment(id = 1, date = "Today")))
6361

64-
assertThat(rows.filterIsInstance<DateHeader>().map { it.keyId }).containsExactly(10, 11)
62+
val beforeHeader = before.filterIsInstance<DateHeader>().single()
63+
val afterHeader = after.filterIsInstance<DateHeader>().single()
64+
assertThat(afterHeader).isEqualTo(beforeHeader)
6565
}
6666

6767
private fun comment(id: Long, date: String) = CommentRsUiModel(

0 commit comments

Comments
 (0)