Fix #2344 improve catalog loading spinner #4075
Conversation
There was a problem hiding this comment.
The result is not yet enough visible. I asked @allyoucanmap to provide you more details.
Don't forget the unit test at the end.
| @@ -360,7 +362,7 @@ class Catalog extends React.Component { | |||
| </Form>)} | |||
| footer={this.renderPagination()}> | |||
| <div> | |||
| {this.renderResult()} | |||
| { this.props.loading ? <div className="catalog-results empty"><Spinner spinnerName="circle" noFadeIn overrideSpinnerClassName="spinner"/></div> : this.renderResult() } | |||
There was a problem hiding this comment.
Instead of Spinner we should use Loader from /misc folder to keep the style aligned with other loader.
I suggest to use size 176px similar to the main spinner of Loading MapStore.
Probably it's better to use a different class for the container that describe the loading action:
proposal -> catalog-results-loading instead of catalog-results empty
There was a problem hiding this comment.
@allyoucanmap what about removing the spinner inside the search button?
There was a problem hiding this comment.
I think @allyoucanmap has forgotten to remove it in the snapshot but we agreed it before that I should remove all other spinners in favour of a single visible large spinner
| @@ -360,7 +359,9 @@ class Catalog extends React.Component { | |||
| </Form>)} | |||
| footer={this.renderPagination()}> | |||
| <div> | |||
| {this.renderResult()} | |||
| { this.props.loading ? <div className="catalog-results loading"><Loader size="176" /><h4> | |||
There was a problem hiding this comment.
i would keep the new loading stuff in the renderLoading function
There was a problem hiding this comment.
Initial, it was in dedicated function because it was used in multiple places but currently it is used in only one place. Besides the Loader by itself is already a component and the surrounding container make sense in that place only. What do you think @MV88
There was a problem hiding this comment.
I know it is used only there, but i think is clearer to use a function if the condition of the ternary is true. it is just a matter of reability taste.
{ this.props.loading ? this.renderLoading() : this.renderResult() }
i think this is more readable then your solution. as I said is a matter of code style.
Also renderResult is used only once but we can decide to leave it there in order to shrink the render method
|
@MV88 I have attached the gif with the new behaviour in PR description page under new behaviour |
There was a problem hiding this comment.
- I can see a loading text that stefano did not provided, please remove it.
- one other thing is the alignment, it should be centered vertically and horizontally
@allyoucanmap confirm?
Catalog loading spinned was small and placed in such a way that it was difficult to see it. This commit make it bigger and easy to notice that the catalog is loading
kasongoyo
force-pushed
the
feature/2344
branch
from
bb8bfb1 to
af6c99d
Compare
|
@kaso please, can you backport this to 2019.02.xx stable branch ? |
…s-it#4075) * Fix geosolutions-it#2344 improve catalog loading spinner Catalog loading spinned was small and placed in such a way that it was difficult to see it. This commit make it bigger and easy to notice that the catalog is loading * make the spinner vertical center aligned
* Fix #2344 improve catalog loading spinner Catalog loading spinned was small and placed in such a way that it was difficult to see it. This commit make it bigger and easy to notice that the catalog is loading * make the spinner vertical center aligned
Description
Catalog loading spinner was small and placed in such a way that it was
difficult to see it. This commit make it bigger and easy to notice that
the catalog is loading.
Improve catalog loading spinner
Issues
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x", remove the others)
What is the current behavior? (You can also link to an open issue here)
see #2344
What is the new behavior?
Does this PR introduce a breaking change? (check one with "x", remove the other)
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Other information: