Fixes #30044 - Host Details Page Redesign #7726
Conversation
| script_src: [webpack_server], connect_src: [webpack_server], | ||
| style_src: [webpack_server], img_src: [webpack_server], | ||
| font_src: ["data: #{webpack_server}"], default_src: [webpack_server] | ||
| } |
There was a problem hiding this comment.
Layout/FirstHashElementIndentation: Indent the right brace the same as the start of the line where the left brace is.
|
Issues: #30044 |
amirfefer
changed the title
|
Looks good. Let's just make sure we have appropriate extension points for plugins in place before this is considered the new default. Let's even try to just define a skeleton and that core uses these extension points. |
Of course, it would take some time until this will be the new default, opinionated extension points will be an important part. meanwhile, this new page can exist in our experimental lab for more feedback. |
|
I'd like to see wider tables. Right now it's hard to see an overview of all the host parameters and their values. I think plugins typically also need the horizontal space. REX is the first example that comes to my mind with an overview of recent tasks. Currently there are 3 columns but the job table might actually want to be 3 columns wide. Other plugins might want that too. The question is then whether it should be added at the bottom or rather a set of tabs. Another thing I like about the current page is that I can perform actions. Again, REX is one that might add custom actions. The current drop down is probably not the best UX so that needs consideration. |
|
Rebased + Added a link from the hosts index page (will appear only if the experimental lab setting is on):
|
webpack/assets/javascripts/react_app/components/HostDetails/index.js
Outdated
Show resolved
Hide resolved
webpack/assets/javascripts/react_app/components/HostDetails/HostDetails.scss
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Nice work @amirfefer !
I like the style and feel of the new components,
left some top-level design inline-comments
see also there are some errors in the devtools
| <StatusAlert | ||
| status={response ? response.global_status_label : null} | ||
| /> |
There was a problem hiding this comment.
it feels like this status is taking too much space,
I think it should be on the page header line with the host name, and maybe some smaller way to show the status,
this component looks a bit like our toast notifications alerts.
There was a problem hiding this comment.
The status is going to change to a multiple statuses card (we have more statuses to show, and the goal is to create aggregation card for it)
| <br /> | ||
| <Grid> | ||
| <GridItem span={2}> | ||
| <Title headingLevel="h5" size="2xl"> |
There was a problem hiding this comment.
Title can become really big, can you add ellipsis with tooltip to it? so it would still be a one liner?
There was a problem hiding this comment.
I'm not sure if ellipsis fit titles from UX perspective, we have lots of space to keep it as a one-line (just a css issue)
| <Properties hostData={response} /> | ||
| </GridItem> | ||
| <GridItem style={{ marginLeft: '40px' }} span={3}> | ||
| <ParametersCard paramters={response.all_parameters} /> |
There was a problem hiding this comment.
What's going to be in this parameters section? didn't see it on the original page
There was a problem hiding this comment.
Parameters that are used by this host, but the list can be big, I wonder to move this section as a different tab with a dedicated table. in the details tab, we can have a high-level overview such as - last audits, last reports, last tasks last updated facts(?) and etc.
| <ParametersCard paramters={response.all_parameters} /> | ||
| </GridItem> | ||
| <GridItem style={{ marginLeft: '40px' }} span={3} rowSpan={2}> | ||
| <AuditCard hostName={response.name} /> |
There was a problem hiding this comment.
When shifted down, it moves also the interfaces downwards
There was a problem hiding this comment.
I use the pf4 grid system, so I assume it has some issues.
BTW I added scrolling so cards have max height, above it, a scroll bar will automatically be added
| }; | ||
| return ( | ||
| <> | ||
| <PageSection className="header" variant={PageSectionVariants.light}> |
There was a problem hiding this comment.
There seem to be a lot of extra space on the right side, and a bit crowded on the left side:
This reverts commit f0882a0.
theforeman-bot
added
Needs re-review
Waiting on contributor
and removed
Waiting on contributor
labels
|
@tbrisker the problem I have with this is that it sets a precedent. Anything temporary sticks around and others will copy it. That means at some point there will be a resource that is important and more dynamic but we'll fail on it. If we do this, IMHO it needs a big fat TODO comment saying this is not right and should not be taken as a good practice. |
Let's continue this discussion on the separate pr for the icons implementation. |
| }, [match.params.id, dispatch]); | ||
|
|
||
| useEffect(() => { | ||
| document.body.classList.add('transprenet-border'); |
There was a problem hiding this comment.
This causes navigation to other react pages from the host detail page to remain with this styling. why not set the class on the main component for this page instead?
Also, there's a typo - it should be transparent-border, but wouldn't a more descriptive name be grey-background? is there no pf4 class for setting this background?
There was a problem hiding this comment.
Thanks @amirfefer and all reviewers, since this is experimental page and is in progress initial implementation, i'm going to merge now and we can continue to iterate and improve it in follow-up prs.
|
Looks like RPM builds are now failing, probably some missing dependency : |
This is an early stage and WIP PR of the new host details page redesign.
The page is based on react and is included in our client routing.
Also, it uses only PF4 components.
Short Demo:
