Refactoring filter setup

[Gnito]

Current filter setup is a mess since routing and state handling (for user inputs) is spread out to 3 different container components:

• SearchFilters (primary filters on desktop)
• SearchFiltersPanel (secondary filters on desktop)
• SearchFiltersMobile

Furthermore, creating different versions of generic enum-type filters is difficult since you need to duplicate code on those different containers. (e.g. amenities filter is using SelectMultipleFilter, but it could be configurable instead of fixed to variable names etc.)

marketplace-custom-config.js contains filter setup (and sort by too).

export const filters = [
  {
    id: 'price',
    label: 'Price',
    type: 'PriceFilter',
    group: 'primary',
    queryParamNames: ['price'],
    config: {
      min: 0,
      max: 1000,
      step: 5,
    },
  },
  {
    id: 'category',
    label: 'Category',
    type: 'SelectSingleFilter',
    group: 'secondary',
    queryParamNames: ['pub_category'],
    config: {
      options: [
        { key: 'smoke', label: 'Smoke' },
        { key: 'electric', label: 'Electric' },
        { key: 'wood', label: 'Wood' },
        { key: 'other', label: 'Other' },
      ],
    },
  },
  {
    id: 'view',
    label: 'View',
    type: 'SelectSingleFilter',
    group: 'secondary',
    queryParamNames: ['pub_view'],
    config: {
      options: [
        { key: 'sea', label: 'Sea' },
        { key: 'lake', label: 'Lake' },
        { key: 'forest', label: 'Forest' },
        { key: 'garden', label: 'Garden' },
        { key: 'other', label: 'Other' },
      ],
    },
  },
//etc.

So, every filter are described in an array (to get rendering order) and they have keys

• id: id of the filter
• label: faster customization without en.json setup
• type: represents one of the existing filter components
• group: is this 'primary' or 'secondary' filter
• queryParamNames: describes parameters to be used with API (e.g. price or pub_amenities)
• config extra configuration that the filter component needs.

In addition, sort configuration also exists in the marketplace-custom-config.js.

---

Then all the filter components are added through a single file (SearchPage/FilterComponent.js) and related code is removed from those child-containers (i.e. SearchFilters, SearchFiltersPanel, SearchFiltersMobile). The idea is that when you add a custom filter component (let's say, LongFilter since we don't have filter component for long type), you only need to touch this one file instead of spreading and duplicating code in different UI containers.

In practice, this means that those container components just render filters through props.children and their main tasks are to provide UI and Apply, Reset all, etc. buttons.

In the SearchPage/MainPanel.js, those presentational containers get their children rendered through one generic component (called FilterComponent), which essentially just maps correct filter config, state/submit handling functions, and other props that the UI container expects.

        <SearchFiltersPrimary {/* what ever props the container needs*/}>
          {filterConfig.filter(selectPrimaryFilters).map(config => {
            return (
              <FilterComponent
                key={`SearchFiltersPrimary.${config.id}`}
                idPrefix="SearchFilters"
                filterConfig={config}
                urlQueryParams={urlQueryParams}
                initialValues={this.initialValues}
                getHandleChangedValueFn={this.getHandleChangedValueFn}
                showAsPopup
                contentPlacementOffset={FILTER_DROPDOWN_OFFSET}
              />
            );
          })}
        </SearchFiltersPrimary>

---

Renaming some of the existing components makes sense at this point:

• SearchFilters -> SearchFiltersPrimary
• SearchFiltersPanel -> SearchFiltersSecondary.

---

SortBy's state is now also tracked in MainPanel - so it's not cleared when filters are updated.

---

Taking update from upstream:
if you have just modified filters on SearchPage, it might be best if you reimplement those filter configurations through marketplace-custom-config.js - i.e. skip the merge conflicts since customizing existing filters (add/remove/update/reorder) is very easy with this new setup.

If you have created completely new filters, you should note that in this new setup initialValues for the filter are strings come originally from URL parameters. Therefore, a filter component needs to parse them themselves and vice versa: onSubmit should give formatted URL parameter values per key (e.g. { price: '15,1000' }).

Because filter configurations have changed, you need to refactor both EditListingWizard (EditListingFeaturesForm and EditListingDescriptionForm) and ListingPage so that select filters get options correctly. Something like this:
https://github.com/sharetribe/ftw-daily/blob/v5.0.0/src/forms/EditListingFeaturesForm/EditListingFeaturesForm.js#L52

[ovan]

@Gnito This looks really good! A couple of comments / questions / suggestions:

1. label - Can I still use translation mechanism to pull in the value for the label if I want to? I guess nothing stopping that since this is a .js file and not a .json? Is that just one require + function call away?
2. paramName - I propose renaming this to queryParam or queryParamName to make it clear how this is used and what it affects.
3. I like the array format to set order. It's very clear and requires no duplication / modifying multiple places.
4. I'm interested to see the handling of has_any / has_all. That's part of the API and spec now so we should support that, and also by default always explicitly set it when building query param values for enum filters instead of relying on server defaults.
5. Is sort defined through the same mechanism, just another filter that sets the sort query params? At least it works a little differently in the sense that you can have multiple sorts but you don't configure the query param to use. Instead you end up compose like: sort=createdAt,-pub_gears,price https://www.sharetribe.com/api-reference/marketplace.html#sorting
6. What about cross filter dependencies? If I use keyword search I want to disable filter control. How would that work in this? For example, <SearchFilters> could take a function that gets called with current composed selections and could, by id (which we'd need to add to filter configuration) disable a filter.

[kpuputti]

IMO this looks good with the features it currently has. I also think that the array to define the order is definitely more convenient than any other hierarchical solution.

Some questions:

1. Is it possible to define filters that have multiple form components? Think about a day picker where the month and day are separate dropdowns. Or is this just an internal detail of the filter component?

2. Is it possible that a single filter controls multiple query params?

3. Can the filter just read the current values of the other filters, i.e. depend on other filters and change its behaviour based on those?

4. Is the query param name supposed to be fully configurable? Currently it kinda is, but not fully. I remember that originally we wanted to support localising the param names, but I don't think that is a common customization. It's a lot clearer to map then directly to the Marketplace API param names.

   I'm mainly asking this since if filters depend on other filter (like the sorting depending on the keyword filter), they need to read the param from the current filter values using the configuration. This is where we currently break away from having fully configurable param names as some places still have the names hard coded.

[Gnito]

@ovan

1. Yes. It should work like that using <FormattedMessage id="some.translation.key.here" />. Current codebase has hard-coded labels in a filter component, but the labels for options are coming from config. This would make the translations of single filter more consistent.

2. Sounds good. (How about even more clear: queryParamNameForAPI?)

3. There are an easy way and a more difficult one:

• Before making the query to API, we just add the correct search type to that particular query parameter
• If we try to use those search types in the filter / client app's URL, we might need to refactor the inner guts of those filters quite much since they use checkboxes' default behaviour to construct URLSearchParameter's value. So, this is potentially a bigger task (I'm not sure if there's any happy surprises that would make this easier.)

5. I was considering leaving the sortConfig in a separate exported variable.

6. If there's any information shared between filters, it would happen in MainPanel. (Those child containers don't care about their own children aka filters in this setup.)

I'm a bit worried about how big a task this is going to be.

• If filter components are responsible to react to this new info, we need to refactor every component. Check for example SortBy component / isKeywordFilterActive.
• We could just show/hide filter if some active filter's config.hideFilters: [id1, id2] states so - but that would also mean that the query params of those disabled filters need to be cleared.

const primaryFilters = filterConfig
  .filter(selectVisibleFilters)
  .filter(selectPrimaryFilters)
  .map(f => <FilterComponent />);

I'm a bit worried about the possible complexity of ´selectVisibleFilters`. To me, it sounds that this kind of "hide these filters" cross-referencing could become cumbersome to deal with (users could create loops there) - which becomes annoying if they start to directly manipulate shared URLs.

[Gnito]

@kpuputti

1. to my understanding, it would be more or less internal issue for that filter (also initialValues / onSubmit needs to be customized accordingly)

2. Good question. I wasn't planning to change your BookingDateRangeFilter, so it should work pretty much similarly as it does with the current setup.

3. This touches the question Olli asked (Q6). To me, it sounds a bad approach to couple filters directly with each others - but customizers have full power to code what they want.

4. I'm not sure we have hard-coded param names other than this:
   https://github.com/sharetribe/ftw-daily/blob/master/src/containers/SearchPage/SearchPage.js#L54

My plan was to get rid of it. So, if we start to reference filters, then we need to add ids to those individual filter configs and find relevant queryParameterName through the config file as discussed in the Olli's Q6. What comes to SortBy config, it would require something like config.disableIfFiltersActive: [id1, id2] => find relevant queryParamNames => convert to props.

[ovan]

@Gnito

5. I was considering leaving the sortConfig in a separate exported variable.

Hmmm. I think the line between filters and sort is muddy so wouldn't it make sense to have them be part of the same system. Like keyword filter is not only a filter but a sort too. Maybe that's not good enough reason though but in any case there are dependencies between the two and that has to be handled somehow.

3. This touches the question Olli asked (Q6). To me, it sounds a bad approach to couple filters directly with each others - but customizers have full power to code what they want.

I too was not thinking that this should be a per filter thing. Like a filter should not need to know about any other filters. I was thinking this would a place above them. A function that gets called with the combined state of all filters (not with a query string but a map keyd by filter id?) before props are passed to filter rendering. A filter would then just take a prop in it's config isDisabled? or similar and all it would have to know how to do is to render itself as disabled or active.

There are an easy way and a more difficult one:
Before making the query to API, we just add the correct search type to that particular query parameter
If we try to use those search types in the filter / client app's URL, we might need to refactor the inner guts of those filters quite much since they use checkboxes' default behaviour to construct URLSearchParameter's value. So, this is potentially a bigger task (I'm not sure if there's any happy surprises that would make this easier.)

I don't think I'm fully following now. The has_all has_any is part of the value the filter pushes up so feels sensible to be able to prepend that to the filter's output value on filter level, right?

This makes me think, what layer is now responsible of taking all the filter output and creating a map that's in passed to the dispatched redux action? I think I just need to see more code on how this is all put together to understand this part so we can also continue this discussion once we're there.

[Gnito]

@ovan If the sort config is among filter configs, then we need to extract it away in all the places where we want to render filters (because SortBy is not rendered in the same place in the UI). This is pretty consistent between any other marketplace and e-commerce sites too - different functionality, so the gestalt theory dictates that it should be separated.

And the exported sort config variable is pretty simple to get it out:

import config from './config';
const { filters, sort } = config.custom;

[Gnito]

Status update:

The key files in this new setup are:

• marketplace-custom-config.js (Current dev version)
• SearchPage/FilterComponent.js (Current dev version)

Through marketplace-custom-config.js one can add/remove/order/configure existing filter components. SelectMultipleFilter and SelectSingleFilter are existing filters that work with extended data (type enum) - so, customizers can add several select filters if they have several enumerable search schemas for listing's extended data.

If there's a need to add completely new filter component (e.g. LongFilter for filtering 'long' type data), customizer needs to create that filter component and add it to the generic filter wrapper: FilterComponent.

Every filter gets initialValues as URIEncoded parameter and they have the responsibility to format their updated values similarly onSubmit. SearchPage/MainPanel.js handles the state of currently modified filters. (I.e. scenario where current URL has a certain state, but a user has modified filters too without clicking "Apply" to actually make new request to fetch listings).

Edit:

• Actual filter components could use some refactoring too, but I'm afraid that's not going to fit to budget.
• SortBy component has a disabled mode (this hasn't changed), but the related conflicting filter is set through sort config. It's just easier to understand limitations if this conflicting setup is mentioned in config file too.
• I haven't tried to include a configurable disabled mode to existing filter components. Basically, this would need UI for every filter and some loop related magic (i.e. what should happen if shared URL contains params that are conflicting with each other. These are also quite case-by-case situations: active sort should not disable keyword search, but active keyword search needs to disable SortBy.)

[Gnito]

In addition to previous comment, we decided to renamed UI containers:
SearchFilters => SearchFiltersPrimary
SearchFiltersPanel => SearchFiltersSecondary
(SearchFiltersMobile stayed the same but some wrongly named translations were updated)

I also added a bug fix to SortBy component's functionality. Now updating a filter doesn't clear sort parameter from the URL (except when keywords filter is active since it is sorting the results already with relevance).

[kpuputti]

I have only gone through the non-component files, but posting this partial review for you to check already.

[kpuputti]

Review part 2, still 10 UI component files to go through.