Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

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

wants to merge 2 commits into from

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

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

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
@@ -210,6 +214,18 @@ class SearchUISearchPage extends React.Component<SearchUISearchPageProps, Search
</div>
</div>
);
const pageContents = showContents ? (
Copy link
Collaborator

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
@@ -260,6 +260,21 @@
exportButtonLabel: 'Export',
},

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

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
@@ -92,7 +93,7 @@ type SearchUISearchPageProps = {
orderHint: Array<string>;
/** Controls the colors used to show various entity types (the value can be any valid CSS color) */
entityColors: Map<string, string>;
/**
/*
Copy link
Collaborator

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

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