MBL-2342: Add search filtering by project location by amy-at-kickstarter · Pull Request #2482 · kickstarter/ios-oss

amy-at-kickstarter · 2025-06-18T15:22:03Z

📲 What

Implement search filtering by location.

location.search.working.mp4

🤔 Why

This is the belated phase 3 of our search filters! Plus filtering by location is very popular.

amy-at-kickstarter · 2025-06-18T20:54:56Z

    self.searchFilters = searchFilters
+
+    let selectedLocationName = searchFilters.location.selectedLocation?.displayableName
+    _locationSearchText = State(initialValue: selectedLocationName ?? "")


I've learned a lot of lessons about initializing state in SwiftUI. You can really only do this particular trick once, when the root view is instantiated.

wow is _locationSearchText like a private accessor to locationSearchText?

Correct, this is a feature of property wrappers (of which State is one). This is what the swift language documentation has to say:

The wrapper defines and manages any underlying storage needed by its wrapped value. The compiler synthesizes storage for the instance of the wrapper type by prefixing the name of the wrapped property with an underscore (_) — for example, the wrapper for someProperty is stored as _someProperty. The synthesized storage for the wrapper has an access control level of private.

So this is the officially supported way to set a State value on init. However, it's pretty tricky to use - if the SwiftUI view is used as a child view anywhere, it may be initialized many times, causing this state to break. This only works here because FilterRootView is initialized once and set as the root view of a UIHostingController.

amy-at-kickstarter · 2025-06-18T20:55:37Z

+      placement: .navigationBarDrawer(displayMode: .always),
+      prompt: "FPO: Search by city, state, country..."
+    )
+    .searchSuggestions {


I spent a bunch of time writing my own autosuggestions code, until eventually I realized Apple had already done it for me 🤷‍♀️

amy-at-kickstarter · 2025-06-18T20:55:55Z

+        selectedLocation: self.$selectedLocation
+      )
+    }
+    .searchable(


This isn't customizable, but I feel like using the system one is very ergonomic and nice.

amy-at-kickstarter · 2025-06-18T20:57:31Z

  func selectedPercentRaisedBucket(_ bucket: DiscoveryParams.PercentRaisedBucket)

+  /// Cal this when the user selects a filter location.
+  func filteredLocation(_: Location?)


Had to name it this everywhere to prevent an overlap with the selectedLocation var.

jovaniks

Overall looks good! Just one suggestion: for any hardcoded strings that don't yet have a corresponding translation, could we wrap them with a TODOS comment or marker so it's easier to track and replace them later?

jovaniks · 2025-06-20T18:33:36Z

  @ViewBuilder
+  var locationSection: some View {
+    FilterSectionButton(
+      title: "FPO: Location",


Do we want to have this on translation strings?

jovaniks · 2025-06-20T18:35:18Z

              .modalHeader(withTitle: Strings.Percentage_raised(), onClose: self.onClose)
+          case .location:
+            self.locationModal
+              .modalHeader(withTitle: "FPO: Location", onClose: self.onClose)


Same here, looks like we want to have this in translated strings

jovaniks · 2025-06-20T18:36:37Z

+        self.selectedLocation = nil
+      } label: {
+        self.buttonLabel(
+          title: "FPO: Anywhere",


jovaniks · 2025-06-20T18:42:19Z

+    .searchable(
+      text: self.$searchText,
+      placement: .navigationBarDrawer(displayMode: .always),
+      prompt: "FPO: Search by city, state, country..."


Would be great to have this included in the translated strings.

jovaniks · 2025-06-20T18:51:29Z

+  var suggestedLocations: [Location]
+  @Binding var selectedLocation: Location?
+  var onSearchedForLocations: (String) -> Void
+  @Binding var searchText: String


Just wondering - since defaultLocations, suggestedLocations, and onSearchedForLocations don't seem to change after initialization, would it make sense to declare them as let instead of var?

Whoops, yes, will fix!

jovaniks · 2025-06-20T18:55:06Z

    self.category.categories = categories
  }

+  internal func update(


Since internal is the default access level in Swift, can we remove it here to avoid redundancy?

jovaniks · 2025-06-20T18:55:15Z

+    self.location.defaultLocations = locations
+  }
+
+  internal func update(


jovaniks · 2025-06-20T18:58:04Z

    )

+    if featureSearchFilterByLocation() {
+      let locationTitle = self.location.selectedLocation?.displayableName ?? "FPO: Location"


Should we replace this with a translated string?

amy-at-kickstarter · 2025-06-20T19:39:14Z

Overall looks good! Just one suggestion: for any hardcoded strings that don't yet have a corresponding translation, could we wrap them with a TODOS comment or marker so it's easier to track and replace them later?

Thanks for the reminder, I'll add TODOs and a ticket number. In this case, "FPO" is "for position only" and is supposed to indicate roughly the same thing, but I do like having TODOs as well.

jovaniks

Awesome

nativeksr · 2025-06-23T20:29:54Z

	1 Warning
⚠️	Big PR

Generated by 🚫 Danger

scottkicks

Sorry for the delay on this one! LGTM

amy-at-kickstarter force-pushed the feat/adyer/location-filter-2 branch 2 times, most recently from 1573761 to 31939b9 Compare June 18, 2025 20:48

amy-at-kickstarter changed the base branch from main to feat/adyer/fetch-locations-use-case June 18, 2025 20:49

amy-at-kickstarter changed the title ~~Feat/adyer/location filter 2~~ MBL-2342: Add search filtering by project location Jun 18, 2025

amy-at-kickstarter commented Jun 18, 2025

View reviewed changes

amy-at-kickstarter force-pushed the feat/adyer/location-filter-2 branch from 31939b9 to 03653ab Compare June 18, 2025 20:56

amy-at-kickstarter commented Jun 18, 2025

View reviewed changes

amy-at-kickstarter marked this pull request as ready for review June 18, 2025 20:59

amy-at-kickstarter requested review from jovaniks and scottkicks June 20, 2025 17:48

jovaniks suggested changes Jun 20, 2025

View reviewed changes

amy-at-kickstarter force-pushed the feat/adyer/fetch-locations-use-case branch from bcb1c3b to e0a6fb0 Compare June 23, 2025 17:29

Base automatically changed from feat/adyer/fetch-locations-use-case to main June 23, 2025 18:25

MBL-2342: Add search filtering by project location

33952f4

amy-at-kickstarter force-pushed the feat/adyer/location-filter-2 branch from 03653ab to 33952f4 Compare June 23, 2025 18:29

Add FIXME for translations

f97af01

amy-at-kickstarter requested a review from jovaniks June 23, 2025 18:31

amy-at-kickstarter added 2 commits June 23, 2025 14:37

Fix another build issues, more comments from the PR

5172cd2

Remove redundant internal access modifier

ae8800f

jovaniks approved these changes Jun 23, 2025

View reviewed changes

Add feature flag override to fix failing test

6314df0

Fetch more default location results

c4368b0

scottkicks approved these changes Jun 24, 2025

View reviewed changes

Merge branch 'main' into feat/adyer/location-filter-2

f917a95

amy-at-kickstarter merged commit f3e487d into main Jun 24, 2025
5 checks passed

amy-at-kickstarter deleted the feat/adyer/location-filter-2 branch June 24, 2025 18:27

amy-at-kickstarter added the sort-and-filter label Jun 26, 2025

Uh oh!

Conversation

amy-at-kickstarter commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📲 What

🤔 Why

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amy-at-kickstarter Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jovaniks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amy-at-kickstarter commented Jun 20, 2025

Uh oh!

jovaniks left a comment

Choose a reason for hiding this comment

Uh oh!

nativeksr commented Jun 23, 2025

Uh oh!

scottkicks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

amy-at-kickstarter commented Jun 18, 2025 •

edited

Loading

amy-at-kickstarter Jun 20, 2025 •

edited

Loading