-
Notifications
You must be signed in to change notification settings - Fork 9
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
[Update] Add category to sample row #226
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.
After tweaking for quite a while, I think I found the shortest code that is the closest to what I imagined. (and am quite proud that I didn't give up 😊 )
- Rotating chevron with non-glitching animation
- No hack to hide the navigation link gray chevron
- Use more native SwiftUI APIs
Even better, it gets rid of the boolean state in the row!
Can you try the code below on all targets, and see if they all behave correctly? Thanks.
Some readings I did
- https://developer.apple.com/documentation/swiftui/disclosuregroup
- https://stackoverflow.com/questions/70396915/how-to-do-a-reveal-style-collapse-expand-animation-in-swiftui
- https://www.appcoda.com/hide-disclosure-indicator-swiftui-list/
- https://gist.github.com/kurtjacobsdev/61b360cd1c86b72cf5f35a6a3ec1ffdb
- https://kristaps.me/blog/swiftui-disclosure-group/
import SwiftUI
struct SampleListView: View {
/// All samples that will be displayed in the list.
let samples: [Sample]
/// A Boolean value indicating whether the row description should include
/// the sample's category. We only need this information when the samples
/// in the list are from different categories.
let shouldShowCategory: Bool // can be passed in from parent view, or computed in place such as Set(samples.map(\.category)).count > 1
var body: some View {
List(samples, id: \.name) { sample in
SampleRow(sample: sample, shouldShowCategory: shouldShowCategory)
}
}
}
private extension SampleListView {
struct SampleRow: View {
/// The sample displayed in the row.
let sample: Sample
/// A Boolean value that indicates whether to show the sample's category.
let shouldShowCategory: Bool
var body: some View {
DisclosureGroup {
VStack(alignment: .leading) {
if shouldShowCategory {
Text("Category: \(sample.category)")
.bold()
}
Text(sample.description)
.foregroundColor(.secondary)
}
.listRowSeparator(.hidden)
.font(.caption)
} label: {
NavigationLink(sample.name) {
SampleDetailView(sample: sample)
}
}
}
}
}
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.
Let's see what others think
I'm okay with closing #170 in favor of using |
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.
General question: Have we considered displaying the description by default with no control over showing/hiding it?
Subtitles/captions/descriptions under list items seem to be a common pattern throughout the OS (recent call entries in Phone, Notes, Messages, Reminders, etc..).
Yea, Ting and I have talked about it. I think it would clutter the searching view too much. The difference between the Sample View and apps like Messages is we are displaying two things, the category and the description, while they are only displaying one. Also, some of the descriptions can be three lines long, which is a lot to display. @dfeinzimer |
It would probably make sense to limit the description display to |
Another idea I have would be to swap the |
I proposed the same thing in a different thread. |
Co-authored-by: David Feinzimer <dfeinzimer@gmail.com>
Looking through Apple's apps, such as the Mail app, I have found the |
I know I've seen it (not necessarily in a list view) but I can't recall where. It may have been a 3rd party non-Apple app. I did some searching around the HIG and found something that seems relevant:
Granted this doesn't depict items in a sidebar but nonetheless seems to recommend against using disclosures to reveal info in lists. |
The joy of UI design: this bit in the HIG seems to contradict the above: Disclosure triangles I'm OK with the Disclosure Triangle. It seems more appropriate than the "info" button. |
I had a thought. Instead of putting the category in each row, we could group the search results by category, putting all of the results for each category in a different section with the category name as the section header. |
I am not sure that would work with the search bar improvements PR that I have up. #228 breaks up the search results into three sections, Name, Description, and Tags, based on how the results were found. Were you hoping to get rid of that design in favor of category sections? @philium |
I hand't seen that other design. But if the search result row shows all three things (the title, the description, and the tags), then they could still be grouped into sections by category. Rows could be similar to the Reminders app, which shows a title, notes, and tags: I'm not saying make it look exactly the same, but have those same elements in that order. |
Initially I asked Caleb to add the category information to the search result rows for the following reason: in the old app, when I search a sample by name, I sometimes cannot figure out which category it belongs to (especially for Maps and Layers samples). Next time when I want to find the sample again, I have to search again, or browse through all the categories. With the addition of this information, now I can nail down the category more quickly with the first search. And next time I don't need to search again. As you may tell, the category info is quite trivial, thus I suggested to only include it in the search results, and hide it in a disclose-able view. Putting them into separate sections might make this info too prominent; displaying the category and the description by default might also put too much visual saliency on these info with less importance. Apart from the category info, other changes look good. |
After discussion with Divesh, we determined that adding the category row is not needed because it would not be of much use to the users. Updating the info button will happen under a different issue/PR. Thank you to everyone that gave feedback on this issue. |
Description
This PR adds the category to the sample row in the Sample Viewer. This allows the user to easily find the category of a sample when searching by hitting the description button on the sample row. The description button was also updated from an info symbol to a chevron due to a discussion that can be found in the issue's comments.
Linked Issue(s)
swift/issues/4298
How To Test
Screenshots