-
-
Notifications
You must be signed in to change notification settings - Fork 125
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
1607 add llm genrated tag #1655
base: main
Are you sure you want to change the base?
1607 add llm genrated tag #1655
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
.ais-HierarchicalMenu-list .ais-HierarchicalMenu-list--child { | ||
display: block; | ||
display: inline-block; | ||
overflow-y: visible; | ||
margin: 0; | ||
padding: 0 0 0 0rem; | ||
width: 100%; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For each item under .ais-HierarchicalMenu-list--child, could you add padding to the left, so we can differentiate between the parent and child tags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current version is close to what UI team design, but I also think add padding will help the readability.
Also one thing to note, I think the styling is affecting the author RefinementList under Browse Testimony, so we might have to add another className to make sure the styles apply only to Browse Bill? |
Good Call! It is because I put the HierarchicalMenu in useRefinement. I should create another useHierarchical and combine two filter when creating the page. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks pretty solid - a few notes with one noticeable bug I think should be addressed before we merge.
Repro Steps: (Using Firefox)
- Filter by a subtopic
- Click on a Bill Result to navigate to that bill's detail page
- Hit "Back" in your browser
Results:
- The Hierarchical filter seem to get added to the search state hundreds of times
tsconfig.json
Outdated
@@ -14,7 +14,8 @@ | |||
"resolveJsonModule": true, | |||
"isolatedModules": true, | |||
"jsx": "preserve", | |||
"incremental": true | |||
"incremental": true, | |||
"downlevelIteration": true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OOC, why is this necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is for iteration, but it is not necessary I can rewrite the code!
@@ -30,6 +31,31 @@ const searchClient = new TypesenseInstantSearchAdapter({ | |||
} | |||
}).searchClient | |||
|
|||
const extractLastSegmentOfRefinements = (items: any[]) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Could we put a more specific type on items
than any
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I feel like we could put a more specific name on this function since it doesn't apply to all refinements? It's specifically for cleaning up the labels for HierarchicalMenu refinements - so maybe something more like displayLowestTierForHierarchical
/ extractLowestTierForHierarchical
?
@@ -30,6 +31,31 @@ const searchClient = new TypesenseInstantSearchAdapter({ | |||
} | |||
}).searchClient | |||
|
|||
const extractLastSegmentOfRefinements = (items: any[]) => { | |||
return items.map(item => { | |||
console.log(item) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: We should probably remove this console.log
before merging
const extractLastSegmentOfRefinements = (items: any[]) => { | ||
return items.map(item => { | ||
console.log(item) | ||
if (item.label != "topics.lvl1") return item |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker, just curious: Is there a type field on item
that would identify hierarchical facets so we could make this more generic? (e.g. if item
's type is actually something like CurrentRefinementsConnectorParamsRefinement
, it would have a type
enum field that has a hierarchical
option - we'd have to test to see if that applies in our case, but it'd be preferable to not have to call out specific fields if it's a straightforward adjustment).
Summary
A functional LLM tags Hierarchical filter
Checklist
A new Instantsearch Facet that filters on Tags
This new content should be toggleable by a new flag in FeatureFlags
Screenshots
Known issues
To improve the functionality we can add search bar to the filer.
Steps to test/reproduce
Go to the browse bill page.
Click on "+" to expand the tag
Click on subtitle to do the filter and check if tag shows on the top of search result.