Conversation
| */ | ||
| showEmptyFacets: boolean; | ||
| /** Facets to not render */ | ||
| hiddenFacets: Array<string>; |
There was a problem hiding this comment.
Is this part of the Tabs change? Is it here so that the facet being used for the tabs doesn't need to be rendered as a separate facet?
There was a problem hiding this comment.
Yes, exactly.
| hide360Link?: boolean; | ||
| /** Whether or not to show tabs based on a facet */ | ||
| showTabs: boolean; | ||
| /** Facet field to use for tabs */ |
There was a problem hiding this comment.
Mention in the comment that the default field is "table."
Alternatively, it might make sense to not have two things to set and just require the user to set the tabsField property to the field they want. If they don't set the field, then there are no tabs; this way the showTabs property isn't needed and it's a little simpler.
I might even change the property name from tabsField to tabsFacetField... Then the comment could say "The facet field to use for splitting the list of search results into tabs. Must be a facet that's present in the query request. If not set, all search results are displayed in a single list."
I might even mention that the tabs here will correspond to the buckets returned for the given facet, just to make it explicit.
You might want to add a note that the user might want to also add the facet field listed here to the hiddenFacets property of the FacetResults component so they aren't in 2 places...
| /** Facet field to use for tabs */ | ||
| tabsField: string; | ||
| /** List of explicit tabs to always show. If empty, tab values will be dynamic based on query response. */ | ||
| tabList: Array<string>; |
There was a problem hiding this comment.
Maybe change the property name to something that includes the idea of them being always there... like fixedTabs?
Also, I think it should be clear that 1) this only applies if tabs are being requested at all and 2) that if there are more facet buckets than the items in this list, they will be shown in addition to these.
| tables = tableFacet.buckets.map((bucket: SearchFacetBucket) => { | ||
| const innerContents = activeTableTabKey === bucket.filter ? results : ''; | ||
| return ( | ||
| <Tab eventKey={bucket.filter} title={`${bucket.value} (${bucket.count})`}> |
There was a problem hiding this comment.
Instead of bucket.value, you might want to use the SearchFacetBucket class' displayLabel() method which will (theoretically) do a better job at figuring out what the user wants to see...
| let innerContents; | ||
| let eventKey = tableName; | ||
| if (tableFacet && tableFacet.buckets) { | ||
| tableFacet.buckets.forEach((facet: SearchFacetBucket) => { |
There was a problem hiding this comment.
Again, I'd use find() here.
| // @flow | ||
| import React from 'react'; | ||
| import PropTypes from 'prop-types'; | ||
| import { Tabs, Tab } from 'react-bootstrap'; |
There was a problem hiding this comment.
Is there a reason to use the bootstrap and directly instead of using a component (the same thing that's used when there are child documents from a join...)?
There was a problem hiding this comment.
@luigivaldez I'm not sure what the pros and cons are of using the react-bootstrap components vs the SUIT components (I assume you're talking about TabPanel/TabInfo? Is the "standard" to use those over the bootstrap ones?
| if (tables && tables.length > 0) { | ||
| const innerContents = activeTableTabKey === 'All' ? results : ''; | ||
| tables.unshift( | ||
| <Tab eventKey="All" title="All"> |
There was a problem hiding this comment.
I might use a different (more complex/unusual) value for the key here, on the off chance that one of the bucket's is called "All"
| {innerContents} | ||
| </Tab>, | ||
| ); | ||
| // ` |
There was a problem hiding this comment.
What's this comment for?
2 participants
Add ability to use tabs to filter results