Skip to content

Comments

issue-107: Modify Searcher to keep track of when a search is being performed and update BusyIndicator component#106

Open
nikhilj13 wants to merge 7 commits intoattivio:masterfrom
nikhilj13:show-spinner-while-searching
Open

issue-107: Modify Searcher to keep track of when a search is being performed and update BusyIndicator component#106
nikhilj13 wants to merge 7 commits intoattivio:masterfrom
nikhilj13:show-spinner-while-searching

Conversation

@nikhilj13
Copy link
Contributor

@nikhilj13 nikhilj13 commented Apr 18, 2019

Adds a new state variable isSearching to the searcher to keep track of when the system is searching. Also updates the BusyIndicator component to have a 'spinnyCentered' type spinner that can be shown when a search is being performed.

Below is an example of BusyIndicator with type 'spinnyCentered' being used in SearchUI:
Spinner

BusyIndicator is configurable through SearchUI's configuration.properties file. Here is an example:

  BusyIndicator: {
	// type of BusyIndicator that should be shown - available types are elipsis, spinny and spinnyCentered 
	// spinnyCentered shows a big spinner in the center of its container with a message
	type: 'spinnyCentered',
	// name of the spinner image with the extension placed in searchui/frontend/src/img folder
	spinner: 'img/spinner.gif',
	// height for the spinner
	spinnerHeight: '50%',
	// width for the spinner
	spinnerWidth: '50%',
	// text that should be displayed with the spinner
  	message: 'Loading Results...',
  },```

- Added a new state isSearching in Searcher.js to keep track of when a search is being done
- Added a new Spinner component
updated class name
@nikhilj13 nikhilj13 changed the title Add a new Spinner Component and changes to Searcher to keep track of when a search is being performed Add a new Spinner Component and made changes to Searcher to keep track of when a search is being performed Apr 18, 2019
@nikhilj13 nikhilj13 changed the title Add a new Spinner Component and made changes to Searcher to keep track of when a search is being performed Added a new Spinner Component and made changes to Searcher to keep track of when a search is being performed Apr 18, 2019
@nikhilj13 nikhilj13 changed the title Added a new Spinner Component and made changes to Searcher to keep track of when a search is being performed Add a new Spinner Component and modify Searcher to keep track of when a search is being performed. Apr 18, 2019
@nikhilj13 nikhilj13 changed the title Add a new Spinner Component and modify Searcher to keep track of when a search is being performed. issue-107: Add a new Spinner Component and modify Searcher to keep track of when a search is being performed. Apr 18, 2019
- Added displayName to Spinner
- Added Spinner to the styleguide
Copy link
Collaborator

@luigivaldez luigivaldez left a comment

Choose a reason for hiding this comment

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

Does the BusyIndicator component not do what you wanted this to do?
https://attivio.github.io/suit/styleguide/index.html#busyindicator
If you need to be able to use a custom image, can you change the existing one to take a property?

type SearcherProps = {
location: PropTypes.object.isRequired;
history: PropTypes.object.isRequired;
location: PropTypes.object.isRequired,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are all of these semicolons changed into commas? Was this your formatter issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I'm not getting any lint errors because of this

return facetFilters.map((facetFilter: FacetFilter): string => {
return `${facetFilter.facetName},+${facetFilter.bucketLabel},+${facetFilter.filter}`;
});
return facetFilters.map(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto here and in deserializeFacetFilter() below... it'll be good to revert this formatting change which is unnecessary, to make it simpler to do merges.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll revert it.

this.props.history.push(`?${updatedQueryString}`);
}
});
const localCallBack = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was going to say "Can you name this something more descriptive?" Although, I think it's clearer what you're doing if you just include this function inline as the second parameter to setState(). For example:

this.setState({
  isSearching: true,
}, () => {
  // start the search...
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, makes sense

@nikhilj13 nikhilj13 changed the title issue-107: Add a new Spinner Component and modify Searcher to keep track of when a search is being performed. issue-107: Modify Searcher to keep track of when a search is being performed and update BusyIndicator component Apr 19, 2019
Copy link
Collaborator

@luigivaldez luigivaldez left a comment

Choose a reason for hiding this comment

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

See inline comments.... Will add comments to the Search UI side of this too...

if (state.debug !== this.props.debug) {
basicState.debug = state.debug;
}
basicState.isSearching = this.state.isSearching;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it makes sense to save the isSearching flag in the URL...

return (
<div className="attivio-spinner">
{this.renderBusySymbol()}
<br />
Copy link
Collaborator

Choose a reason for hiding this comment

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

If there's no messageToShow, you probably don't want to add these <br>s and the message at all.

import Configurable from './Configurable';
import spinnyImage from '../img/spinner.gif';

type BusyIndicatorType = 'ellipsis' | 'spinny';
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to add 'spinnyCentered' here...

@klk1010 klk1010 added the merge conflicts This PR has a merge conflict that needs to be resolved. label Sep 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge conflicts This PR has a merge conflict that needs to be resolved.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants