Conversation
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 29181 | |
| Version | PR #24868 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 71ad107 | |
| Installation URL | 0ri9m2dt4tkpo |
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 29181 | |
| Version | PR #24868 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 71ad107 | |
| Installation URL | 1gqf83s0o52o0 |
kean
force-pushed
the
task/improve-top-list-performance
branch
from
4ab9eb6 to
228f8a1
Compare
| Group { | ||
| if viewModel.isFirstLoad { | ||
| listContent(data: mockData()) | ||
| .redacted(reason: .placeholder) |
There was a problem hiding this comment.
It does not feel right to implement skeleton UI using mock data. It looks fine on screen, but using mock data in production just doesn't feel right.
There was a problem hiding this comment.
It does not feel right to implement skeleton UI using mock data.
Can you elaborate why? That's how every screen with .redacted(reason: .placeholder) does it. What's the alternative?
There was a problem hiding this comment.
The mockData reads the test data from memory or the app bundle, and then it's used to build the view. But I imagine you don't actually need to use any real content, just random text and numbers would be fine too?
Eventually, we are going to remove the mock data, whether it's the hard-coded ones or the ones in the bundle. And when we remove them, we'd need to change this mockData() usage to be something else. I guess that's what I meant, "does not feel" right.
I'm not too bothered, though. It's okay if you'd like to keep the change. We'll cross that bridge when we come to it.
There was a problem hiding this comment.
There are two separate "mock data" types in Stats. There mock data in MockStatsService that is elaborate, covers multiple years and is based on JSON files. And then there is small in-memory TopListData.mock designed specifically to use in placeholders and SwiftUI previews for quick testing. It's basically arbitrary text and numbers.
There was a problem hiding this comment.
Right. I probably thought of them as one thing. 👍
| static let mostDownloadeded = AppLocalizedString("jetpackStats.topList.mostDownloads", value: "Most Downloaded", comment: "Title for chart") | ||
|
|
||
| static func top(_ count: Int) -> String { | ||
| String.localizedStringWithFormat(AppLocalizedString("jetpackStats.postDetails.topN", value: "Top %1$d", comment: "Top N, e.g. 'Top 10'"), count) |
There was a problem hiding this comment.
In English, "Top 10" makes much more sense than "Top ten". But in Chinese, the difference between "前十名" and "前10名" is more subtle. The former is more formal, but both can be used as translations. I'm not sure about other languages, though. Given that, maybe we should translate "Top 10" and "Top 50", instead of "Top x"?
kean
force-pushed
the
task/improve-top-list-performance
branch
from
228f8a1 to
d44923d
Compare
|
3 participants
Changes
Listrenders lazily, has better animations, and by showing only top 50 items we improve it further.Screenshots
The screen now only shows 50 items by default. I assume most users will never scroll past that. If they really need to see more, there is a "Show More" button.
To make the list a bit more interesting and complex, I tried showing the top items in a "Top 10" lists with numbering. I think it turned out well.