Skip to content
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

feat: add show only framed works filter #15108

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { ArtworkLocationFilter } from "Components/ArtworkFilter/ArtworkFilters/A
import { AttributionClassFilter } from "Components/ArtworkFilter/ArtworkFilters/AttributionClassFilter"
import { AvailabilityFilter } from "Components/ArtworkFilter/ArtworkFilters/AvailabilityFilter"
import { ColorFilter } from "Components/ArtworkFilter/ArtworkFilters/ColorFilter"
import { FramedFilter } from "Components/ArtworkFilter/ArtworkFilters/FramedFilter"
import { KeywordFilter } from "Components/ArtworkFilter/ArtworkFilters/KeywordFilter"
import { MaterialsFilter } from "Components/ArtworkFilter/ArtworkFilters/MaterialsFilter"
import { MediumFilter } from "Components/ArtworkFilter/ArtworkFilters/MediumFilter"
Expand All @@ -14,13 +15,17 @@ import { PriceRangeFilter } from "Components/ArtworkFilter/ArtworkFilters/PriceR
import { SizeFilter } from "Components/ArtworkFilter/ArtworkFilters/SizeFilter"
import { TimePeriodFilter } from "Components/ArtworkFilter/ArtworkFilters/TimePeriodFilter"
import { WaysToBuyFilter } from "Components/ArtworkFilter/ArtworkFilters/WaysToBuyFilter"
import { useFeatureFlag } from "System/Hooks/useFeatureFlag"
import { useSystemContext } from "System/Hooks/useSystemContext"

type ArtistArtworkFiltersProps = {}

export const ArtistArtworkFilters: React.FC<
React.PropsWithChildren<ArtistArtworkFiltersProps>
> = props => {
const enableShowOnlyFramedArtworksFilter = useFeatureFlag(
"onyx_only_framed_artworks_filter",
)
Comment on lines +26 to +28
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need the feature flag? In case something goes wrong, we cand simply roll back the feature.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I'm in favor of the feature flag. Gives us the ability to do more fine-grained QA (in staging, or per user) before launching. I love the velocity of Hackathon but I appreciate rolling things out intentionally like this 😄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, the feature flag gives more flexibility for the release (and frees us from having to follow the bug-free programming principle 😄).

const { user } = useSystemContext()

return (
Expand All @@ -40,6 +45,7 @@ export const ArtistArtworkFilters: React.FC<
<TimePeriodFilter />
<ColorFilter />
<PartnersFilter />
{enableShowOnlyFramedArtworksFilter && <FramedFilter />}
</Join>
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { ArtworkLocationFilter } from "Components/ArtworkFilter/ArtworkFilters/A
import { AttributionClassFilter } from "Components/ArtworkFilter/ArtworkFilters/AttributionClassFilter"
import { AvailabilityFilter } from "Components/ArtworkFilter/ArtworkFilters/AvailabilityFilter"
import { ColorFilter } from "Components/ArtworkFilter/ArtworkFilters/ColorFilter"
import { FramedFilter } from "Components/ArtworkFilter/ArtworkFilters/FramedFilter"
import { MaterialsFilter } from "Components/ArtworkFilter/ArtworkFilters/MaterialsFilter"
import { MediumFilter } from "Components/ArtworkFilter/ArtworkFilters/MediumFilter"
import { PartnersFilter } from "Components/ArtworkFilter/ArtworkFilters/PartnersFilter"
Expand All @@ -19,6 +20,7 @@ import { SizeFilter } from "Components/ArtworkFilter/ArtworkFilters/SizeFilter"
import { TimePeriodFilter } from "Components/ArtworkFilter/ArtworkFilters/TimePeriodFilter"
import { WaysToBuyFilter } from "Components/ArtworkFilter/ArtworkFilters/WaysToBuyFilter"
import { updateUrl } from "Components/ArtworkFilter/Utils/urlBuilder"
import { useFeatureFlag } from "System/Hooks/useFeatureFlag"
import { useRouter } from "System/Hooks/useRouter"
import { useSystemContext } from "System/Hooks/useSystemContext"
import { __internal__useMatchMedia } from "Utils/Hooks/useMatchMedia"
Expand All @@ -41,6 +43,9 @@ interface CollectionArtworksFilterProps {
export const CollectionArtworksFilter: React.FC<
React.PropsWithChildren<CollectionArtworksFilterProps>
> = props => {
const enableShowOnlyFramedArtworksFilter = useFeatureFlag(
"onyx_only_framed_artworks_filter",
)
const { relay, collection, aggregations, counts } = props
const { slug, query } = collection
const isArtistCollection = query?.artistIDs?.length === 1
Expand All @@ -64,6 +69,7 @@ export const CollectionArtworksFilter: React.FC<
<TimePeriodFilter expanded />
<ColorFilter expanded />
<PartnersFilter expanded />
{enableShowOnlyFramedArtworksFilter && <FramedFilter expanded />}
</Join>
)

Expand Down
7 changes: 7 additions & 0 deletions src/Apps/Search/Components/SearchResultsArtworksFilters.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,21 @@ import { ArtworkLocationFilter } from "Components/ArtworkFilter/ArtworkFilters/A
import { AttributionClassFilter } from "Components/ArtworkFilter/ArtworkFilters/AttributionClassFilter"
import { AvailabilityFilter } from "Components/ArtworkFilter/ArtworkFilters/AvailabilityFilter"
import { ColorFilter } from "Components/ArtworkFilter/ArtworkFilters/ColorFilter"
import { FramedFilter } from "Components/ArtworkFilter/ArtworkFilters/FramedFilter"
import { MaterialsFilter } from "Components/ArtworkFilter/ArtworkFilters/MaterialsFilter"
import { MediumFilter } from "Components/ArtworkFilter/ArtworkFilters/MediumFilter"
import { PartnersFilter } from "Components/ArtworkFilter/ArtworkFilters/PartnersFilter"
import { PriceRangeFilter } from "Components/ArtworkFilter/ArtworkFilters/PriceRangeFilter"
import { SizeFilter } from "Components/ArtworkFilter/ArtworkFilters/SizeFilter"
import { TimePeriodFilter } from "Components/ArtworkFilter/ArtworkFilters/TimePeriodFilter"
import { WaysToBuyFilter } from "Components/ArtworkFilter/ArtworkFilters/WaysToBuyFilter"
import { useFeatureFlag } from "System/Hooks/useFeatureFlag"

export const SearchResultsArtworksFilters = () => {
const enableShowOnlyFramedArtworksFilter = useFeatureFlag(
"onyx_only_framed_artworks_filter",
)

return (
<Join separator={<Spacer y={4} />}>
<ArtistsFilter expanded />
Expand All @@ -29,6 +35,7 @@ export const SearchResultsArtworksFilters = () => {
<TimePeriodFilter expanded />
<ColorFilter expanded />
<PartnersFilter expanded />
{enableShowOnlyFramedArtworksFilter && <FramedFilter expanded />}
</Join>
)
}
3 changes: 2 additions & 1 deletion src/Components/Alert/AlertProvider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -94,11 +94,12 @@ export const AlertProvider: FC<React.PropsWithChildren<AlertProviderProps>> = ({
if (isEditMode || isAlertArtworksView) return
const criteria = getAllowedSearchCriteria(initialCriteria ?? {})

// `forSale` is allowed as a filter criterion,
// `forSale` and `framed` are allowed as a filter criterion,
// but NOT as an alert criterion, so we remove it.
// (Alerts, by definition, stipulate forSale=true
// when they are created in Gravity.)
delete criteria.forSale
delete criteria.framed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought (non-blocking): I think this makes sense for now. Maybe a product decision about whether this would be a desirable alert criteria, but we can always follow up on that.


dispatch({ type: "SET_CRITERIA", payload: criteria })
}, [initialCriteria, isEditMode, isAlertArtworksView])
Expand Down
33 changes: 22 additions & 11 deletions src/Components/ArtworkFilter/ArtworkFilterContext.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -15,24 +15,24 @@ import { rangeToTuple } from "./Utils/rangeToTuple"
* Initial filter state
*/
export const initialArtworkFilterState: ArtworkFilters = {
majorPeriods: [],
page: 1,
sizes: [],
sort: "-decayed_merch",
additionalGeneIDs: [],
artistIDs: [],
artistNationalities: [],
artistSeriesIDs: [],
attributionClass: [],
keyword: undefined,
partnerIDs: [],
additionalGeneIDs: [],
colors: [],
height: "*-*",
keyword: undefined,
locationCities: [],
artistNationalities: [],
majorPeriods: [],
materialsTerms: [],
height: "*-*",
width: "*-*",
priceRange: "*-*",
metric: DEFAULT_METRIC,
page: 1,
partnerIDs: [],
priceRange: "*-*",
sizes: [],
sort: "-decayed_merch",
width: "*-*",
}

/**
Expand All @@ -47,6 +47,7 @@ export enum FilterParamName {
attributionClass = "attributionClass",
colors = "colors",
forSale = "forSale",
framed = "framed",
height = "height",
keyword = "keyword",
locationCities = "locationCities",
Expand Down Expand Up @@ -138,6 +139,7 @@ export enum SelectedFiltersCountsLabels {
attributionClass = "attributionClass",
colors = "colors",
forSale = "forSale",
framed = "framed",
locationCities = "locationCities",
materialsTerms = "materialsTerms",
medium = "medium",
Expand Down Expand Up @@ -483,6 +485,7 @@ const artworkFilterReducer = (
"acquireable",
"atAuction",
"forSale",
"framed",
"includeArtworksByFollowedArtists",
"inquireableOnly",
"offerable",
Expand Down Expand Up @@ -533,6 +536,7 @@ const artworkFilterReducer = (
"atAuction",
"color",
"forSale",
"framed",
"includeArtworksByFollowedArtists",
"inquireableOnly",
"offerable",
Expand Down Expand Up @@ -648,6 +652,13 @@ export const getSelectedFiltersCounts = (
break
}

case paramName === FilterParamName.framed: {
if (paramValue) {
counts[paramName] = 1
}
break
}

default: {
counts[paramName] = 1
}
Expand Down
1 change: 1 addition & 0 deletions src/Components/ArtworkFilter/ArtworkFilterTypes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export interface ArtworkFilters extends MultiSelectArtworkFilters {
atAuction?: boolean
color?: string
forSale?: boolean
framed?: boolean
height?: string
includeArtworksByFollowedArtists?: boolean
inquireableOnly?: boolean
Expand Down
44 changes: 44 additions & 0 deletions src/Components/ArtworkFilter/ArtworkFilters/FramedFilter.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import { Checkbox } from "@artsy/palette"
import {
SelectedFiltersCountsLabels,
useArtworkFilterContext,
useCurrentlySelectedFilters,
} from "Components/ArtworkFilter/ArtworkFilterContext"
import { FilterExpandable } from "Components/ArtworkFilter/ArtworkFilters/FilterExpandable"
import { useFilterLabelCountByKey } from "Components/ArtworkFilter/Utils/useFilterLabelCountByKey"

interface FramedFilterProps {
expanded?: boolean
}

export const FramedFilter: React.FC<FramedFilterProps> = ({ expanded }) => {
const { unsetFilter, setFilter } = useArtworkFilterContext()
const currentSelectedFilters = useCurrentlySelectedFilters()

const filtersCount = useFilterLabelCountByKey(
SelectedFiltersCountsLabels.framed,
)
const label = `Framed${filtersCount}`

const isSelected = currentSelectedFilters?.framed

const handleOnSelect = (selected: boolean) => {
if (selected) {
setFilter("framed", true)
} else {
unsetFilter("framed")
}
}

return (
<FilterExpandable label={label} expanded={isSelected || expanded}>
<Checkbox
selected={!!currentSelectedFilters?.framed}
onSelect={handleOnSelect}
my={1}
>
Only framed works
</Checkbox>
</FilterExpandable>
)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
import { screen } from "@testing-library/react"
import userEvent from "@testing-library/user-event"
import { useArtworkFilterContext } from "Components/ArtworkFilter/ArtworkFilterContext"
import { FramedFilter } from "Components/ArtworkFilter/ArtworkFilters/FramedFilter"
import {
createArtworkFilterTestRenderer,
currentArtworkFilterContext,
} from "Components/ArtworkFilter/ArtworkFilters/__tests__/Utils"
import { __internal__useMatchMedia } from "Utils/Hooks/useMatchMedia"
import { useEffect } from "react"

jest.mock("Utils/Hooks/useMatchMedia")

const render = createArtworkFilterTestRenderer()

describe(FramedFilter, () => {
it("renders a toggle", () => {
render(<FramedFilter expanded />)
expect(screen.getByText("Only framed works")).toBeInTheDocument()
})

it("updates context on filter change", () => {
render(<FramedFilter expanded />)
expect(currentArtworkFilterContext().filters?.framed).toBeFalsy()

userEvent.click(screen.getAllByRole("checkbox")[0])
expect(currentArtworkFilterContext().filters?.framed).toBeTruthy()

userEvent.click(screen.getAllByRole("checkbox")[0])
expect(currentArtworkFilterContext().filters?.framed).toBeNull()
})

it("clears local input state after Clear All", () => {
render(<FramedFilter expanded />)
userEvent.click(screen.getAllByRole("checkbox")[0])
expect(currentArtworkFilterContext().filters?.framed).toBeTruthy()

userEvent.click(screen.getByText("Clear all"))

expect(currentArtworkFilterContext().filters?.framed).toBeUndefined()
})

describe("mobile-specific behavior", () => {
beforeEach(() => {
// Simulate the media query that checks for xs size and returns true
;(__internal__useMatchMedia as jest.Mock).mockImplementation(() => true)

/*
* A test component that simulates the usage of
* the FramedFilter inside ArtworkFilterMobileOverlay
*/
const MobileVersionOfFramedFilter = () => {
const filterContext = useArtworkFilterContext()

// biome-ignore lint/correctness/useExhaustiveDependencies: <explanation>
useEffect(() => {
// on mount, initialize the staged filters
filterContext.setShouldStageFilterChanges?.(true)
if (filterContext.filters) {
filterContext.setStagedFilters?.(filterContext.filters)
}
}, [])

return <FramedFilter expanded />
}
render(<MobileVersionOfFramedFilter />)
})

afterEach(() => {
jest.clearAllMocks()
})

it("stages the filter change instead of applying", () => {
expect(currentArtworkFilterContext().filters?.framed).toBeFalsy()

userEvent.click(screen.getAllByRole("checkbox")[0])

expect(currentArtworkFilterContext().filters?.framed).toBeFalsy()
expect(currentArtworkFilterContext().stagedFilters?.framed).toBeTruthy()
})

it("displays a filter count inline", () => {
expect(screen.getByText("Framed")).toBeInTheDocument()
expect(screen.queryByText("Framed • 1")).not.toBeInTheDocument()

userEvent.click(screen.getAllByRole("checkbox")[0])

expect(screen.getByText("Framed • 1")).toBeInTheDocument()
})
})

describe("the `expanded` prop", () => {
it("hides the filter controls when not set", () => {
render(<FramedFilter />)
expect(screen.queryAllByRole("checkbox").length).toBe(0)
})

it("hides the filter controls when `false`", () => {
render(<FramedFilter expanded={false} />)
expect(screen.queryAllByRole("checkbox").length).toBe(0)
})

it("shows the filter controls when `true`", () => {
render(<FramedFilter expanded />)
expect(screen.queryAllByRole("checkbox").length).not.toBe(0)
})
})
})
6 changes: 6 additions & 0 deletions src/Components/ArtworkFilter/ArtworkFilters/index.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { Join, Spacer } from "@artsy/palette"
import { AvailabilityFilter } from "Components/ArtworkFilter/ArtworkFilters/AvailabilityFilter"
import { FramedFilter } from "Components/ArtworkFilter/ArtworkFilters/FramedFilter"
import { KeywordFilter } from "Components/ArtworkFilter/ArtworkFilters/KeywordFilter"
import { useFeatureFlag } from "System/Hooks/useFeatureFlag"
import type * as React from "react"
import { ArtistNationalityFilter } from "./ArtistNationalityFilter"
import { ArtistsFilter } from "./ArtistsFilter"
Expand All @@ -23,6 +25,9 @@ interface ArtworkFiltersProps {
export const ArtworkFilters: React.FC<
React.PropsWithChildren<ArtworkFiltersProps>
> = props => {
const enableShowOnlyFramedArtworksFilter = useFeatureFlag(
"onyx_only_framed_artworks_filter",
)
const { user } = props

return (
Expand All @@ -41,6 +46,7 @@ export const ArtworkFilters: React.FC<
<TimePeriodFilter expanded />
<ColorFilter expanded />
<PartnersFilter expanded />
{enableShowOnlyFramedArtworksFilter && <FramedFilter expanded />}
</Join>
)
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ describe("urlFragmentFromState", () => {
}

expect(urlFragmentFromState(artworkFilterState)).toEqual(
"height=300-400&width=500-600&priceRange=100-200",
"height=300-400&priceRange=100-200&width=500-600",
)
})
})
Loading