feat: multi-selection time period filters

[iskounen]

The type of this PR is: Feature

This PR resolves FX-2624

This PR is the work of @dzucconi, @dblandin, and @anandaroop

Description

• Allows multiple selection of time period (a.k.a. major period) filters on artwork grids.

Screenshots

Before

After

PR Checklist (tick all before merging)

• I have included screenshots or videos to illustrate my changes, or I have not changed anything that impacts the UI.
• I have added tests for my changes, or my changes don't require testing, or I have included a link to a separate Jira ticket covering the tests.
• I have documented any follow-up work that this PR will require, or it does not require any.
• I have added an app state migration, or my changes do not require one. (What are migrations?)
• I have added a CHANGELOG.yml entry or my changes do not require one.

[iskounen]

The Apply button is currently displaying "Apply (1)" for any combination of time periods because we pass all selected options as one array of values like so:

[{"displayText": "2010-2019", "paramName": "majorPeriods", "paramValue": ["2020", "2010"]}]

The "Ways to buy" filter, however, can display "Apply (2)" when two options are selected because it passes each selected option as a separate filter like so:

[{"displayText": "Bid", "paramName": "atAuction", "paramValue": true}, {"displayText": "Make offer", "paramName": "offerable", "paramValue": true}]

[MounirDhahri]

This looks good.

About the designs, I thought we were trying to push more into the direction of using checkmarks instead of toggle switches to have more consistent behavior in all filter types. It offers a better UX (since the user will expect to check an item by pressing anywhere in the row instead of only selecting the toggle switch). But I see that the filters specs made by CX are designed differently than the ones made by FX. @samchieng do you mind if you clear this out to make sure we don't have more different type of filters in the future.

The Apply button is currently displaying "Apply (1)" for any combination of time periods because we pass all selected options as one array of values like so:

That's great, that's the behavior we want.

The "Ways to buy" filter, however, can display "Apply (2)" when two options are selected because it passes each selected option as a separate filter like so:

Ideally we should be showing this as 1 as well. Take a look at this

eigen/src/lib/Components/FilterModal/FilterModal.tsx

Lines 177 to 187 in ac5ac0a

	// For Sale Artworks, the artistsIDs and the includeArtworksByFollowedArtists filters behave like one
	if (state.filterType === "saleArtwork") {
	const hasArtistsIFollow = !!state.selectedFilters.find(
	(filter) => filter.paramName === FilterParamName.artistsIFollow
	)
	const hasArtistIDs = !!state.selectedFilters.find((filter) => filter.paramName === FilterParamName.artistIDs)
	if (hasArtistIDs && hasArtistsIFollow) {
	--selectedFiltersSum
	}
	}

and this

eigen/src/lib/Components/FilterModal/FilterModal.tsx

Lines 603 to 614 in ac5ac0a

	// For Sale Artworks, the artistsIDs and the includeArtworksByFollowedArtists filters behave like one
	// Therefore we need to decrement the number of filters by one to give the user the impression they are one
	if (state.filterType === "saleArtwork") {
	const hasArtistsIFollow = !!state.appliedFilters.find(
	(filter) => filter.paramName === FilterParamName.artistsIFollow
	)
	const hasArtistIDs = !!state.appliedFilters.find((filter) => filter.paramName === FilterParamName.artistIDs)
	if (hasArtistIDs && hasArtistsIFollow) {
	--selectedFiltersSum
	}
	}

[anandaroop]

Nice, and thanks for leading this mob 🙏

[anandaroop]

Based on real-world data I was expecting to see these as ranges ("2000–2010" etc) — but this is fine bc they match the mock data, yeah?

[iskounen]

That's correct. I confirmed by changing the mocked data to ["Roger", "Robert"] and observing that the result was a comma-separated string "Roger, Robert".

[dblandin]

LGTM 👍 💯

[samchieng]

@MounirDhahri yes, apologies for the inconsistencies right now! Currently collaborating with Tricia to refine the final filter specs.

[iskounen]

This PR will be in a draft state until the work to place it behind a feature flag is complete.

[iskounen]

I didn't put this behind a feature flag because I don't think that the actual value matters for the single-selection implementation, as long as there is one.

I tested the "All" option on the time periods filter with and without the feature flag to confirm this, and both the results and the filters screen behaved as expected.

[dzucconi]

Looks good to me. Feel free to merge after the changelog conflict is resolved 😞