-
Notifications
You must be signed in to change notification settings - Fork 27
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 FacetTabs Component #116
base: master
Are you sure you want to change the base?
Conversation
/** | ||
* An optional list of facets that should not be shown but are needed for other reasons | ||
*/ | ||
hideFacet: Array<string>; |
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.
Add ?
to indicate this is optional. Make the prop name plural to indicate it's an array, not a boolean.
hideFacet: Array<string>; | |
hiddenFacets?: Array<string>; |
@@ -57,6 +62,7 @@ type FacetResultsDefaultProps = { | |||
orderHint: Array<string>; | |||
entityColors: Map<string, string>; | |||
showEmptyFacets: boolean; | |||
hideFacet: Array<string>, |
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.
Rename prop to indicate this is an array not a boolean.
hideFacet: Array<string>, | |
hiddenFacets: Array<string>, |
@@ -81,6 +87,7 @@ export default class FacetResults extends React.Component<FacetResultsDefaultPro | |||
orderHint: [], | |||
entityColors: new Map(), | |||
showEmptyFacets: false, | |||
hideFacet: [], |
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.
hideFacet: [], | |
hiddenFacets: [], |
@@ -129,6 +136,11 @@ export default class FacetResults extends React.Component<FacetResultsDefaultPro | |||
} | |||
|
|||
shouldShow(facet: SearchFacet): boolean { | |||
if (this.props.hideFacet && this.props.hideFacet.length > 0) { |
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.
if (this.props.hideFacet && this.props.hideFacet.length > 0) { | |
if (this.props.hiddenFacets && this.props.hiddenFacets.length > 0) { |
@@ -129,6 +136,11 @@ export default class FacetResults extends React.Component<FacetResultsDefaultPro | |||
} | |||
|
|||
shouldShow(facet: SearchFacet): boolean { | |||
if (this.props.hideFacet && this.props.hideFacet.length > 0) { | |||
if (this.props.hideFacet.includes(facet.field)) { |
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.
if (this.props.hideFacet.includes(facet.field)) { | |
if (this.props.hiddenFacets.includes(facet.field)) { |
(this: any).handleTabClick = this.handleTabClick.bind(this); | ||
} | ||
|
||
componentWillMount() { |
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.
Avoid using componentWillMount
|
||
render() { | ||
const tabData = this.state.tabsList; | ||
const tabs = |
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.
Pull tabs
out into renderTabs
); | ||
} | ||
}); | ||
this.setState({ tabsList: tabs }); |
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.
From react docs, ("Thinking in React")[https://reactjs.org/docs/thinking-in-react.html#step-3-identify-the-minimal-but-complete-representation-of-ui-state]
Ask three questions about each piece of data:
Is it passed in from a parent via props? If so, it probably isn’t state.
Does it remain unchanged over time? If so, it probably isn’t state.
Can you compute it based on any other state or props in your component? If so, it isn’t state.
With that in mind, if you're storing html as state you should reconsider.
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.
Address feedback.
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.
Is there a change to add the style changes to the less file in SUIT too (that the style guide uses)?
/** | ||
* Name of the facet that should be displayed as Tabs, default is 'table' | ||
*/ | ||
facetName: string; //eslint-disable-line |
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.
Why is the eslint-disable-line
needed here?
/** | ||
* Optional callback to be used when a tab is clicked instead of default (which applies the facet filter) | ||
*/ | ||
handleClick?: () => void; |
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.
Shouldn't this callback take information about the facet whose tab is clicked? Perhaps the entire SearchFacetBucket object? How will the callback know what was clicked and what to do?
Oh, I just got down further to where you call this and you are passing the bucket... so you should change the type of this to (bucket: SearchFacetBucket) => void
and also add to the comment so someone using this knows what they need to pass in...
|
||
//eslint-disable-next-line | ||
numberWithCommas(x) { | ||
return x.toString().replace(/\B(?=(\d{3})+(?!\d))/g, ','); |
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.
You can just use x.toLocaleString() to do this without needing a special function.
47626.toLocaleString()
gives 47,626.
|
||
getTabsFromFacet(facet: SearchFacet) { | ||
const tabs = []; | ||
facet.buckets.forEach((bucket: SearchFacetBucket) => { |
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.
Do you need to have a limit on the number of tabs? What if there are 70 buckets?
populateTabs(props: FacetTabsProps) { | ||
const { facetName, facet } = props; | ||
|
||
if (facet && facet.buckets) { |
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.
What happens if the facet had buckets before but doesn't any more (e.g. because of a different search term)? Will the old tabs not be replaced?
const searcher = this.context.searcher; | ||
if (searcher && searcher.state.response) { | ||
if (searcher.state.response.facets) { | ||
searcher.state.response.facets.forEach((currentFacet: SearchFacet) => { |
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.
This is really a find()
operation on the array.... it would be clearer to call find()
instead of forEach()
....
className="facet-tab" | ||
> | ||
<div className="facet-tab-text"> | ||
{value} <span style={{ color: 'gray' }}>({count})</span> |
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.
If you're going to have a CSS style for the tab and the text of the tab, you should use another one for the count instead of hardcoding the gray color here.
{tabData} | ||
</div> | ||
) : ( | ||
<BusyIndicator |
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.
What if there are just no buckets for the facet? I don't think we want to show the spinny in that case...
Adds a FacetTabs component that shows facet as a tab. The main use for facet tabs would be to show the search results based on the data source. Below is an example:
Also modifies FacetResults to take in a list of facets that should be hidden. If we're showing a facet as tabs we most likely would not want the facet to show up in facet results. It is configurable from SearchUI's configuration properties file. Here's an example: