Skip to content

Comments

issue-43: MiniSearchUI can now be integrated with "external" results and components#44

Open
brandon-bogan wants to merge 12 commits intoattivio:masterfrom
brandon-bogan:MiniSearchUIConfigurability
Open

issue-43: MiniSearchUI can now be integrated with "external" results and components#44
brandon-bogan wants to merge 12 commits intoattivio:masterfrom
brandon-bogan:MiniSearchUIConfigurability

Conversation

@brandon-bogan
Copy link
Contributor

MiniSearchUI can now take optional props for results to render. These props, in one way or another, are propagated on to SearchResultsCount and SearchResults, and so these components were also updated as part of this new feature.

MiniSearchUI also takes props for functions to call when the user enters a query and/or clicks search, so that the results being passed in can be updated accordingly.

* Optional callback to handle searching if not using the context's
* Searcher component to do the searching (otherwise the searcher's onSearch method is used).
*/
onSearch: null | (q: string) => void;
Copy link
Contributor

@klk1010 klk1010 Jul 26, 2019

Choose a reason for hiding this comment

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

Indicate this is optional: onSearch?: null | (q: string) => void;

The difference between
A: onSearch: null | (q: string) => void; and
B: onSearch?: null | (q: string) => void;

In example A, you are specifying that the user must explicitly pass a prop named onSearch and it may have the value of null or it could be a function like (q: string) => void. Omitting a function is not the same as null. If onSearch was not passed as a prop, it would be undefined.

Same goes for updateQuery, response, and errror.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Made onSearch, updateQuery, response, and error optional props. I also removed the null option for onSearch and updateQuery and removed these props from the default props, since these should either be valid functions or not exist at all as props.


type MiniSearchUIDefaultProps = {
scale: number;
onSearch: null | (q: string) => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unlike MiniSearchUIProps, MiniSearchUIDefaultProps is explicitly defined in your component. defaultProps are based on your specifications on lines 60-66 and are not based on what the parent passes through as a prop. Keeping that in mind, onSearch, updateQuery, response, and error, can only be null here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Made all defaultProps null, removed onSearch and updateQuery from defaultProps.

showScores: boolean;
showTags: boolean;
showRatings: boolean;
response: QueryResponse | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

As a defaultProp, response in this instance can only be null.

* A response can be passed in from custom searches if you don't want
* to use the response on the searcher in this.context
*/
response: QueryResponse | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be made optional for backwards compatibility.
response?: QueryResponse | null;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

*/
response: QueryResponse | null;
/** Offset of search results for long scrolling or pagination */
offset: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be made optional for backwards compatibility.
offset?: number;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

const response = searcher.state.response;
const offset = searcher.state.resultsOffset;
const response = this.props.response !== null ? this.props.response : searcher.state.response;
const offset = this.props.response ? this.props.offset : searcher.state.resultsOffset;
Copy link
Contributor

Choose a reason for hiding this comment

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

Make offset optional for backwards compatibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to:
const offset = this.props.response && this.props.offset ? this.props.offset : searcher.state.resultsOffset;

Is this what you meant?


static displayName = 'SearchResultsCount';

render() {
Copy link
Contributor

@klk1010 klk1010 Jul 26, 2019

Choose a reason for hiding this comment

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

Here's an example of a more functional approach. It follows immutable principles so you never modify props. It also cleans up the main render by moving the logic out into a new function.

renderMessage = () => {
  const { response, error } = this.props;
  const { searcher } = this.context;
  const derivedResponse = response ? response : searcher.state.response | null;

  let message;
  let countMessage;

  if (response) {
    const { totalHits: count } = response;
    if (count === 0 || typeof count !== 'number') {
      countMessage = 'No results found';
    } else if (count === 1) {
      countMessage = '1 result found';
    } else {
      countMessage = `${count} results found`;
    }
    return (
      <span>
        {countMessage}
        <span style={{ fontWeight: 'normal' }}>
          {' '}
          (in {response.totalTime}ms)
        </span>
      </span>
    );
  } else if (error) {
    return `Error: ${error}`;
  } else if (searcher.state.error) {
      // got an error...
    return `Error: ${searcher.state.error}`;
  } else {
    // Not yet searched...
    return ''; 
  }
}

render() {
  return (
    <div className="attivio-globalmastnavbar-results">
      {this.renderMessage()}
    </div>
  );
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Note that I wrote the 3rd line of renderMessage slightly different to avoid a lint error but otherwise incorporated your code suggestion above.

Copy link
Contributor

@klk1010 klk1010 left a comment

Choose a reason for hiding this comment

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

Address feedback.

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.

4 participants