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

Add facets to AppliedFilters #43

Merged
merged 10 commits into from
Oct 22, 2021
Merged

Add facets to AppliedFilters #43

merged 10 commits into from
Oct 22, 2021

Conversation

yen-tt
Copy link
Contributor

@yen-tt yen-tt commented Oct 21, 2021

  • update DisplayableFilter interface to include additional fields to help identify and unify the different applied filter types (static, nlp, facet)
  • update functions in filterUtils to support applied facets

J=1656
TEST=manual

  • in vertical page, input query 'virginia', and select some static filters and facet options, see that all 3 label types are displayed

J=1656
TEST=manual
@coveralls
Copy link

coveralls commented Oct 21, 2021

Coverage Status

Coverage remained the same at 74.713% when pulling 8903573 on dev/facets-in-appliedfilters into c85e3de on main.

Copy link
Contributor

@tmeyer2115 tmeyer2115 left a comment

Choose a reason for hiding this comment

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

The filterutils class is getting pretty complicated with all the different things going on. There's grouping, pruning, collecting, etc. I think we really need to clean this file up. But, that's something outside the scope of what you're doing.

filterGroupLabel: string,
filterLabel: string
filterType: 'NLP_FILTER' | 'STATIC_FILTER' | 'FACET',
fieldId: string,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make things harder if we had the interface as the following:

 filterType: ...,
 filter: Filter,
 count?: number,
 groupLabel: string,
 label: string

When I see an interface called DisplayableFilter, I think it makes sense for it to include a Filter and any additional display metadata for it. I think relating the DisplayableFilter interface to Filter has other advantages as well. It's better re-use of code and were we to change Filter, we would not have to make the same updates to this type as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Additionally, we might consider adding a type param to the Filter interface. That seems like a good fit.

Copy link
Contributor Author

@yen-tt yen-tt Oct 22, 2021

Choose a reason for hiding this comment

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

part of the main purpose of having DisplayableFilter is so it can unify the structure of different filters (AppliedQueryFilter, Filter, and DisplayableFacets - doesn't use Filter), Otherwise, it would be more tricky and messy to perform comparison and/or extract information from different applied filter types just pulling from the filter field. actually nvm, that makes sense, I didn't interpret your comment right initially. Updated!

sample-app/src/utils/filterutils.tsx Outdated Show resolved Hide resolved
sample-app/src/utils/filterutils.tsx Outdated Show resolved Hide resolved
sample-app/src/models/displayableFilter.ts Outdated Show resolved Hide resolved
sample-app/src/models/displayableFilter.ts Show resolved Hide resolved
sample-app/src/utils/filterutils.tsx Outdated Show resolved Hide resolved
sample-app/src/utils/filterutils.tsx Outdated Show resolved Hide resolved
@@ -1,20 +1,28 @@
import { AppliedQueryFilter, CombinedFilter, Filter } from '@yext/answers-core';
import { AppliedQueryFilter, CombinedFilter, Filter, DisplayableFacet, NearFilterValue } from '@yext/answers-core';
Copy link
Contributor

Choose a reason for hiding this comment

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

I have an idea for how we could re-organize this file. I think, conceptually, it would be simpler if we had a file that only operated on the DisplayableFilter level of abstraction. That file would contain the pruning and grouping.

We'd have another file that was meant to convert the filters, facets, and applied query filters into DisplayableFilters.

I think this makes for a better separation of concerns. We don't have one file spanning multiple levels of abstraction. Right now this file has broadly two responsibilities: generating a list of DisplayableFilters and then modifying it. We should delegate the first responsibility to methods in another file. Those methods then provide the initial list that methods in this class will pair down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made an update to reorganize filterutils file:

  1. filterutils file: contains only standard Filter functions (i.e. isDuplicateFilter, isCombinedFilter, flattenFilters)
  2. displayablefilterutils file: functions that convert filters, facets, and applied query filters into DisplayableFilters.
  3. appliedfilterurils file: pruning, grouping, etc. that is use for the purpose of spitting out an grouped filter array for appliedfilter

@yen-tt yen-tt requested a review from cea2aj October 22, 2021 16:21
@yen-tt yen-tt merged commit 0e09edc into main Oct 22, 2021
@yen-tt yen-tt deleted the dev/facets-in-appliedfilters branch October 22, 2021 16:24
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.

None yet

4 participants