-
Notifications
You must be signed in to change notification settings - Fork 30
Conversation
@@ -8,51 +9,57 @@ import { | |||
DashboardCardTitle, | |||
} from '../../Dashboard/DashboardCard'; | |||
import { ClusterOverviewContextGenericConsumer } from '../ClusterOverviewContext'; | |||
import { Loading } from '../../Loading'; |
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.
The loading component still has blank-slate-pf
style inside. I would suggest to make two component <Loading>
and <InlineLoading>
Pull Request Test Coverage Report for Build 1050
💛 - Coveralls |
@suomiy @mareklibra I have issues with cosmos fixtures as it seems Provider for some reason does not work correctly, it does not pass props and always returns undefined. Any ideas ? It also does not work on master, every component just renders loading which is due to defaultProps. https://kubevirt.io/web-ui-components/?component=p&fixture=ClusterOverview |
You have to wrap the fixture props in value:
|
src/components/Loading/Loading.js
Outdated
export const InlineLoading = ({ text, id }) => ( | ||
<div id={id} key={prefixedId(id, 'progress') || 'progress'}> | ||
<div className="spinner spinner-lg blank-slate-pf-icon" /> | ||
{text && <h3 className="blank-slate-pf-main-action">{text}</h3>} |
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.
we probably do not want h3 text inside the InlineLoading. I do not think the text is necessary at all.
also, not sure if it would make sense to have a span instead of a div.
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.
or 2nd option: having the text inline
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.
text is now removed and I also made spinner size configurable
Thanks! I was expecting that it will be something small and fishy :) |
</DashboardCardBody> | ||
</DashboardCard> | ||
); | ||
|
||
Details.defaultProps = { | ||
loaded: false, | ||
LoadingComponent: Loading, |
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.
not sure if it is not published yet, but please use the InlineLoading
component here
293c15c
to
7c1b4a9
Compare
if (result) { | ||
let item = result; | ||
if (Array.isArray(result)) { | ||
[item] = result; |
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.
can we use more obvious syntax here, please?
Sort of
item = result[0]
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.
const item = Array.isArray(result) ? result[0] : result
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.
@mareklibra updated
Add loading state to Details item
Create Cluster Details Card
Add loading state to Details item