Skip to content

Conversation

@Mudaafi
Copy link
Contributor

@Mudaafi Mudaafi commented Mar 29, 2024

Pull Request Checklist

Before you submit a pull request, please make sure you have to following:

  • I have added or updated TypeScript types for my changes, ensuring they are compatible with the existing codebase.
  • I have added JSDoc comments to my TypeScript definitions for improved documentation.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added any necessary documentation (if appropriate).
  • I have made sure my PR is up-to-date with the main branch.

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no API changes)
  • Documentation content changes
  • TypeScript type definitions update
  • Other... Please describe:

@Mudaafi Mudaafi added the wip Edits in Progress label Mar 29, 2024
@linear
Copy link

linear bot commented Mar 29, 2024

CSL-2954 [MVP] Create a useFilters Hook (Only Search)

Business Outcome:

  • As a Consumer I want Filters to be handled OOO so that I can fully port over my current implementation to use Constructor's OS UI Library

Definition of done:

  • This hook should exist within the context of the PlpContext
  • Takes in a Search Response object and returns the relevant fields and functions required to power filtering
  • Tests
  • Documentation

Additional notes:

  • You can look at pagination story as an example here
  • We want to handle both facets and groups

@Mudaafi Mudaafi removed the wip Edits in Progress label Apr 26, 2024
@Mudaafi Mudaafi requested a review from a team April 26, 2024 15:57
const applyFilter = (facetGroupName: string, facetValue: any) => {
const newFilters = requestFilters || {};

newFilters[facetGroupName] = facetValue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this apply multiple values properly or would it override the existing one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would not, but I was thinking of having whatever is calling the applyFilter function take care of that (i.e. passing a range, array of values, etc). wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's okay but I feel like we need to be more explicit with the name of the function and documentation. It might be just me but applyFilter makes me think this will append filters rather than replacing them. Maybe something like setFilter? Doesn't have to be set if we can come up with something better. Also side note now that I'm a Typescript nerd, is the facetValue really any or can we make it more strict?

@Mudaafi Mudaafi requested a review from esezen May 14, 2024 23:41
Copy link
Contributor

@esezen esezen left a comment

Choose a reason for hiding this comment

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

LGTM! One small comment about a type now that I am a TS nerd 🤓

setRequestConfigs,
} = useRequestConfigs();

const setFilter = (filterName: string, filterValue: any) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can also change this any to PlpFilterValue, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yessir, thanks for catching that

@Mudaafi Mudaafi merged commit ac52f89 into main May 17, 2024
@Mudaafi Mudaafi deleted the csl-2954-mvp-create-a-usefilters-hook-only-search branch May 17, 2024 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants