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: Enhance DetailsCard for new fields #2586
dashboard: Enhance DetailsCard for new fields #2586
Conversation
cc @andybraren , @rawagner |
You should rebase on top of #2520 which changes |
@mareklibra @rawagner I think update channel is something that might also be required for the Storage dashboard also. |
Rebased on top of #2520 |
@@ -89,6 +88,14 @@ export const DetailsCard_ = connect(mapStateToProps)(({ | |||
<DetailsBody> | |||
{openshiftFlag ? ( | |||
<> | |||
<DetailItem | |||
key="apiurl" |
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 you remove key
prop for all Items ? its actually not needed
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.
done
key="apiurl" | ||
title="Cluster API address" | ||
isLoading={!infrastructureLoaded && !infrastructureError} | ||
error={!infrastructurePlatform} |
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.
shouldnt it be error={!!infrastructureError || !getInfrastructureAPIURL(infrastructureData)}
?
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.
done
key="updatechannel" | ||
title="Update channel" | ||
isLoading={!clusterVersionLoaded && !clusterVersionError} | ||
error={!!clusterVersionError} |
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'd add || !getCVChannel(clusterVersionData)
after adding it, can you extract the getCVChannel(clusterVersionData)
so we call it just once for error and children ?
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.
done
Yay! Thank you @mareklibra! 😄
Good idea! We have some ideas for 4.3 around initiating upgrades from the dashboard that may affect this, but if this PR is for 4.2 this sounds great. 👍 |
Rebased as #2520 is merged now |
rebased |
Rebased to |
/lgtm |
/assign @kyoto |
/approve |
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
/retest Please review the full test history for this PR and help us cut down flakes. |
Good call @ncameronbritt, thank you! The nearby Status card's message will include a call-to-action to "Update", so reusing the exact same CTA here makes sense, especially since they both lead to the same modal. Sorry for so many last minute tweaks @mareklibra 🙂 - could "Update available" be changed to just "Update"? |
@andybraren , @ncameronbritt , the action is renamed now. |
/retest |
Rebased |
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.
Thank you @mareklibra!
@spadgett , can you please have a look? |
isLoading?: boolean; | ||
error?: boolean; | ||
children: React.ReactNode; | ||
valueClass?: string; |
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.
Nit: valueClassName
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.
+1 to valueClassName
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.
done
LGTM (with a small nit) #2586 (review) Not giving |
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.
Thanks, this looks cleaner to me
<Button | ||
variant="link" | ||
className="btn-link--no-btn-default-values" | ||
onClick={() => history.push('/settings/cluster/')} |
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.
Make this a link instead of a button since it's a link :)
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.
done
isLoading?: boolean; | ||
error?: boolean; | ||
children: React.ReactNode; | ||
valueClass?: string; |
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.
+1 to valueClassName
@spadgett , review comments addressed, can you please have a look? |
LGTM on recent change diff. |
@spadgett , wdyt, please? |
Added: - Cluster API address - Update channel Changed: - Cluster ID contains link to Cluster Manager - OpenShift Cluster Version can pop-up a modal dialog to update cluster Generic changes: - Textual dashboard detail items are rendered with the "co-select-to-copy" to simplify copy&paste user action
Rebased |
/hold cancel |
/lgtm |
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.
/approve
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andybraren, kyoto, mareklibra, rawagner, spadgett The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
/retest Please review the full test history for this PR and help us cut down flakes. |
3 similar comments
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest |
/retest Please review the full test history for this PR and help us cut down flakes. |
Last test flake should be fixed by #3158 /retest |
Added:
Changed:
Generic changes:
Depends on: