Skip to content

Comments

suit-issue-107: Use updated BusyIndicator from SUIT #75

Open
nikhilj13 wants to merge 2 commits intoattivio:developfrom
nikhilj13:show-spinner-while-searching
Open

suit-issue-107: Use updated BusyIndicator from SUIT #75
nikhilj13 wants to merge 2 commits intoattivio:developfrom
nikhilj13:show-spinner-while-searching

Conversation

@nikhilj13
Copy link
Contributor

@nikhilj13 nikhilj13 commented Apr 18, 2019 •
edited by klk1010
Loading

Uses the updated BusyIndicator Component from SUIT on SearchUISearchPage and Document360Page. Also adds css for SUIT's updated BusyIndicator Component, once

Below is a screen capture 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...',
  },```

@nikhilj13
Used Spinner
3fe71ac 
- Used the new Spinner Component from SUIT on SearchUISearchPage and Document360Page
- Added css for SUIT's Spinner Component in attivio-spinner.less
@nikhilj13 nikhilj13 changed the title Used Spinner from SUIT suit-issue-107: Use Spinner from SUIT Apr 18, 2019
luigivaldez
luigivaldez reviewed Apr 18, 2019
View reviewed changes
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.

I think it would be great if, ideally, the spinner only appeared after a bit of a delay. That way, if a search is very quick, there's not flash when the spinner is rendered and then quickly replaced by the results. Thoughts?

@nikhilj13
Copy link
Contributor Author

nikhilj13 commented Apr 19, 2019

I think it would be great if, ideally, the spinner only appeared after a bit of a delay. That way, if a search is very quick, there's not flash when the spinner is rendered and then quickly replaced by the results. Thoughts?
Ideally, it would be great if you could configure the minimum search time threshold before the spinner appears (for eg. set to 500 milliseconds by default) but even then you might get a flash if the search takes 1 second. Also, you can't really notice the flash if the search is that fast.

@nikhilj13
Used BusyIndicator in place of Spinner
f60f27d 
Also fixed formatting issues
@nikhilj13 nikhilj13 changed the title suit-issue-107: Use Spinner from SUIT suit-issue-107: Use updated BusyIndicator from SUIT Apr 19, 2019
luigivaldez
luigivaldez requested changes Jul 16, 2019
View reviewed changes
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

frontend/src/pages/SearchUISearchPage.js
</Masthead>
</div>
);
const mainContents = (
Copy link
Collaborator

@luigivaldez luigivaldez Jul 16, 2019

Choose a reason for hiding this comment

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

It seems weird for me that the secondary nav bar should go away while the search is going on...

frontend/src/pages/SearchUISearchPage.js
</div>
</div>
);
const pageContents = showContents ? (
Copy link
Collaborator

@luigivaldez luigivaldez Jul 16, 2019

Choose a reason for hiding this comment

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

Is there a way for this to look less disruptive? When I click Go I will see the page go blank except for the spinner and then show the results...
What do you think of just drawing the spinny thing on top of a translucent gray overlay while keeping the previous results semi-visible until the new ones are fetched?

frontend/configuration.properties.js
},

// These properties configure the spinner that is shown when a search is being performed.
BusyIndicator: {
Copy link
Collaborator

@luigivaldez luigivaldez Jul 16, 2019

Choose a reason for hiding this comment

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

I'm not 100% convinced that the best thing to do in general is to set these in the configuration instead of just setting them on the two instances of the component... but I guess I don't care too strongly about it...1

frontend/src/pages/SearchUISearchPage.js
/** Controls the colors used to show various entity types (the value can be any valid CSS color) */
entityColors: Map<string, string>;
/**
/*
Copy link
Collaborator

@luigivaldez luigivaldez Jul 16, 2019

Choose a reason for hiding this comment

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

This was intentionally a JSDoc ( /** */) comment so it appears in the documentation...

frontend/src/style/bootstrap/attivio-spinner.less
position: fixed;
top: 50%;
left: 50%;
transform: translate(-50%, -50%);
Copy link
Collaborator

@luigivaldez luigivaldez Jul 16, 2019

Choose a reason for hiding this comment

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

It seems from this page that transform: translate() is supported in all of the browsers w support... if so, the various vendor-specific values shouldn't be necessary...
https://developer.mozilla.org/en-US/docs/Web/CSS/transform-function

@klk1010 klk1010 added the merge conflicts Has merge conflicts that need to be resolved label Jul 22, 2019
@klk1010 klk1010 changed the base branch from master to develop July 26, 2019 19:11
@klk1010 klk1010 added this to the v1.0.8 milestone Jul 26, 2019
@klk1010 klk1010 added the dependent Dependent on another PR/release label Sep 18, 2019
@klk1010 klk1010 modified the milestones: v5.6.3, 5.6.5 Oct 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@luigivaldez luigivaldez luigivaldez requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

dependent Dependent on another PR/release merge conflicts Has merge conflicts that need to be resolved

Projects

None yet

Milestone

5.6.5

Development

Successfully merging this pull request may close these issues.

3 participants

@nikhilj13 @luigivaldez @klk1010