-
Notifications
You must be signed in to change notification settings - Fork 294
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
Fixes #38013 - Add Image mode card on host details tab #11219
base: master
Are you sure you want to change the base?
Conversation
1e5e0a0
to
2566eca
Compare
I think it'd be cool to have our shiny new image mode icon on the card :) |
@sambible I think this card might be a good candidate to use as a stopping point for packit testing on what we've developed so far for image mode. |
2566eca
to
60ced84
Compare
60ced84
to
ada9f5c
Compare
Icon looks awesome :) but we maybe don't need so many! I was thinking just in the header. Maybe instead of "Image information" it could be " Image-mode host". |
ada9f5c
to
5c0a156
Compare
1f08eaa
to
f8b982f
Compare
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.
Working great! Just a couple suggestions.
const imageMode = hostDetails?.content_facet_attributes?.bootc_booted_image; | ||
const header = ( | ||
<> | ||
<span style={{ marginRight: '0.5rem' }}>{__('Image mode host')}</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.
"Image mode host" as a title more sounds like it's announcing that this is an image mode host rather than describing image mode host details.
What if we change it to:
<span style={{ marginRight: '0.5rem' }}>{__('Image mode host')}</span> | |
<span style={{ marginRight: '0.5rem' }}>{__('Image mode details')}</span> |
<Dt>{__('Running image digest')}</Dt> | ||
<Dd>{hostDetails.content_facet_attributes.bootc_booted_digest}</Dd> | ||
|
||
<Dt>{__('Staged image')}</Dt> |
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.
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 seems to work well:
diff --git a/webpack/components/extensions/HostDetails/DetailsTabCards/ImageModeCard.js b/webpack/components/extensions/HostDetails/DetailsTabCards/ImageModeCard.js
index 7ec258bf77..ce281d6274 100644
--- a/webpack/components/extensions/HostDetails/DetailsTabCards/ImageModeCard.js
+++ b/webpack/components/extensions/HostDetails/DetailsTabCards/ImageModeCard.js
@@ -19,6 +19,8 @@ const ImageModeCard = ({ isExpandedGlobal, hostDetails }) => {
</>
);
if (!imageMode) return null;
+ const getValueOrDash = value => (value || '-');
+
return (
<CardTemplate
header={header}
@@ -29,24 +31,24 @@ const ImageModeCard = ({ isExpandedGlobal, hostDetails }) => {
<DescriptionList isHorizontal>
<DescriptionListGroup>
<Dt>{__('Running image')}</Dt>
- <Dd>{hostDetails.content_facet_attributes.bootc_booted_image}</Dd>
+ <Dd>{getValueOrDash(hostDetails.content_facet_attributes.bootc_booted_image)}</Dd>
<Dt>{__('Running image digest')}</Dt>
- <Dd>{hostDetails.content_facet_attributes.bootc_booted_digest}</Dd>
+ <Dd>{getValueOrDash(hostDetails.content_facet_attributes.bootc_booted_digest)}</Dd>
<Dt>{__('Staged image')}</Dt>
- <Dd>{hostDetails.content_facet_attributes.bootc_staged_image}</Dd>
+ <Dd>{getValueOrDash(hostDetails.content_facet_attributes.bootc_staged_image)}</Dd>
<Dt>{__('Staged image digest')}</Dt>
- <Dd>{hostDetails.content_facet_attributes.bootc_staged_digest}</Dd>
+ <Dd>{getValueOrDash(hostDetails.content_facet_attributes.bootc_staged_digest)}</Dd>
<Dt>{__('Available image')}</Dt>
- <Dd>{hostDetails.content_facet_attributes.bootc_available_image}</Dd>
+ <Dd>{getValueOrDash(hostDetails.content_facet_attributes.bootc_available_image)}</Dd>
<Dt>{__('Available image digest')}</Dt>
- <Dd>{hostDetails.content_facet_attributes.bootc_available_digest}</Dd>
+ <Dd>{getValueOrDash(hostDetails.content_facet_attributes.bootc_available_digest)}</Dd>
<Dt>{__('Rollback image')}</Dt>
- <Dd>{hostDetails.content_facet_attributes.bootc_rollback_image}</Dd>
+ <Dd>{getValueOrDash(hostDetails.content_facet_attributes.bootc_rollback_image)}</Dd>
<Dt>{__('Rollback image digest')}</Dt>
- <Dd>{hostDetails.content_facet_attributes.bootc_rollback_digest}</Dd>
+ <Dd>{getValueOrDash(hostDetails.content_facet_attributes.bootc_rollback_digest)}</Dd>
</DescriptionListGroup>
</DescriptionList>
</CardTemplate>
f8b982f
to
1667dd8
Compare
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 is looking perfect to me.
I'd like to see if we can get a review from @sambible since we've merged a few imagae mode things without a check from a QE reviewer. I'll hold off on acking the PR for now.
Looks great except I'd make the icon a bit bigger (and move it up vertically a bit so it's better aligned.) |
const header = ( | ||
<> | ||
<span style={{ marginRight: '0.5rem' }}>{__('Image mode details')}</span> | ||
<FontAwesomeImageModeIcon /> | ||
</> | ||
); |
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 could be moved outside the component so it doesn't get recreated on each render. Not essential here though, it shouldn't be too expensive :)
</> | ||
); | ||
if (!imageMode) return null; | ||
const getValueOrDash = value => (value || '-'); |
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 not actually a hyphen ;)
const getValueOrDash = value => (value || '-'); | |
const getValueOrDash = value => (value || '—'); |
oh! Yeah, that's much better.. |
1667dd8
to
167b9a1
Compare
What are the changes introduced in this pull request?
Add Image mode card on host details tab for bootc hosts
Considerations taken when implementing this change?
Depends on #11209
What are the testing steps for this pull request?
Follow the test steps in #11209.
Go to a bootc host - details tab.