Open
Conversation
This comment has been minimized.
This comment has been minimized.
455b961 to
803e9f0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reviewing #1770 I was finding that there was a good opportunity to make a shared
FilterViewfor both iOS and tvOS. Right now, on tvOS, there isn't a way to call this but this sets that up for success as we build out #1770 further so that item can be more manageable for review.Please confirm that I am correct in my assumptions here first. If I am incorrect there, this PR is not required and we can close this out.
While working on the
FilterView, I also addresses an outstanding TODO to allow multiple filters on the sameFilterView. This changes theinitto take[ItemFilterType]instead of a singleItemFilterType. This loops over eachItemFilterTypeto build it out in the same way this was built prior with the exception that we are using section headers when there are multiple filters (see screenshots). Additionally, the bindings are created inside theselectorView(for:)function.Currently, I am not using multiple filters on the same view since I think that will require a bit of a rework of the
navigationBarFilterDrawerand I wanted to keep this a little more focused than I normally do.Other Cleanup
SelectorViewwas the same as theListRowCheckbox. I've just added an optional parameter to remove the O when the item was not selected. Functionally, there are no changes but this lets us re-use the sizing for the checkmark so we don't end up with any variations in that over time.NavigationBarCloseButtonto tvOS to prevent the build errors when sharing a view with tvOS. This also enables this as an option moving forward if we want it. If we don't want it, I think this makes more sense to put a placeholder instead. Similar to...navigationBarTitleDisplayModeto prevent build issues forPlatformViews.ItemFilterTypesystemImageable. I also made itIdentifiableto help with theForEach.Outstanding Questions
FilterView, I am currently making the titletypes.map(\.displayTitle).joined(separator: " & ")which works for 2 filters well (see screenshots) but more filters this would be an issue. What would be the correct title for this compositeFilterView?FilterViewon tvOS, I am currently getting thesystemImagefrom the first element with a fallback to"line.3.horizontal.decrease". What would be the appropriate way to get theSplitFormWindowView.descriptionViewvalue? I had an idea of making a grid of all elements'systemImagebut that seemed like a lot so I thought I would ask first.selectorView(for:)in returns "None" (as this does prior to this PR) but do we want this to show as "None" for a secondary filter on a compositeFilterViewas well? Or would we rather have the empty filters just not show on a compositeFilterViewso long as another filter has values?Screenshots
iOS
Single Filter
Composite Filters
tvOS
Single Filter
Composite Filters