-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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.
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
[Dashboard bug] Slice doesn't always show loading icon when loading #3834
[Dashboard bug] Slice doesn't always show loading icon when loading #3834
Conversation
Coverage remained the same at 71.457% when pulling 284f5f517a7226f71f94b89853744bf169655735 on graceguo-supercat:gg-LoadingCSS into 591e5ec on apache:master. |
@@ -146,7 +146,7 @@ class Chart extends React.PureComponent { | |||
} | |||
|
|||
render() { | |||
const isLoading = this.props.chartStatus === 'loading'; | |||
const isLoading = [undefined, 'loading'].indexOf(this.props.chartStatus) !== -1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this should either be a defaultProps
or handled by the parent. @michellethomas what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isLoading
logic is to show component when fetching query data.
I remember you mentioned want to keep loading
related logic inside Chart
:
ab59c4c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just meant that we souldn't have and handle an undefined
state. My impression is that there should be a defined chartStatus
from the moment the component is created. I'd have to look at the code upstream, but maybe it's a matter of having loading
set when creating the chart object.
I do think that the load image should be shown inside the Chart component though as it is now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now i understand. fixed in the initial Chart state.
284f5f5
to
7ebf256
Compare
34ec8d5
to
0f25ba2
Compare
lgtm |
@michellethomas