-
Notifications
You must be signed in to change notification settings - Fork 255
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
feat(xo-web/render-xo-item): simpler item components #3547
Conversation
cc640b0
to
abbc976
Compare
object: PropTypes.object.isRequired, | ||
return ( | ||
<span> | ||
<Icon icon='network' /> {network.name_label} |
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.
why u delete this:
{pool && `(${pool.name_label || pool.id})`}
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.
It's in the pool component.
return ( | ||
<span> | ||
<Icon icon='disk' /> {vdi.name_label} | ||
{vdi.name_description && <span> ({vdi.name_description})</span>} |
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.
why you didn't display the SR instead description?
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 kept it as it was. I'm not trying to change the behaviour in this PR.
0b3662a
to
77d9882
Compare
77d9882
to
15b930b
Compare
b021910
to
2a45815
Compare
2a45815
to
c8ea6d1
Compare
]) | ||
|
||
SrItem.propTypes = XO_ITEM_PROP_TYPES | ||
VmTemplate.propTypes = { | ||
id: PropTypes.string.isRequired, |
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.
self
property ?
]) | ||
|
||
SrItem.propTypes = XO_ITEM_PROP_TYPES | ||
VmTemplate.propTypes = { | ||
id: PropTypes.string.isRequired, |
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.
self
property ?
const getHost = createGetObject() | ||
return { | ||
host: getHost, | ||
pool: createGetObject(createSelector(getHost, host => host.$pool)), |
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.
host
can be undefined
.
// =================================================================== | ||
|
||
export const Sr = decorate([ | ||
connectStore(() => { |
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.
connectStore(() => {
const getSr = (state, props) => createGetObject()(state, props, props.self)
return {
sr: getSr,
container: createGetObject(
createSelector(
getSr,
(sr = {}) => sr.$container
)
),
}
})
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.
(state, props) => createGetObject()(state, props, props.self)
would create a new selector (thus invalidate the cache) on each call.
RemoteItem.propTypes = XO_ITEM_PROP_TYPES | ||
Sr.propTypes = { | ||
id: PropTypes.string.isRequired, | ||
link: PropTypes.bool, |
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.
self
?
const getObject = createGetObject() | ||
// FIXME: props.self ugly workaround to get object as a self user | ||
return (state, props) => ({ | ||
vdi: getObject(state, props, props.self), |
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.
You can make it 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.
You mean inline getObject
? We want to use the same selector for all the component's lifetime.
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.
My bad, you're right, we don't want create a function on each call.
return (state, props) => ({ | ||
// true to bypass permissions as a self user | ||
sr: getSr(state, props, true), | ||
network: getObject(state, props, props.self), |
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.
same here.
const getObject = createGetObject() | ||
// FIXME: props.self ugly workaround to get object as a self user | ||
return (state, props) => ({ | ||
vdi: getObject(state, props, props.self), |
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.
My bad, you're right, we don't want create a function on each call.
See #2605
Check list
Fixes #007
)${name} v${new version}
)Process
WiP:
(Work in Progress) if not ready to be merged