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
Add List component for detail properties #3889
Add List component for detail properties #3889
Conversation
/assign @suomiy |
@@ -12,15 +12,13 @@ const NodeIPList: React.FC<NodeIPListProps> = ({ ips, expand = false }) => ( | |||
{_.sortBy(ips, ['type']).map( | |||
({ type, address }) => | |||
address && ( | |||
<div key={`{${type}/${address}`} className="co-node-ip"> | |||
<DetailPropertyList key={`{${type}/${address}`}> |
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.
shouldn't this be wrapped arround all ips? and not for each item
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.
Oops, yes, I think the div
was here by mistake as well as it should not render when the address is not supposed to render.
@@ -0,0 +1,12 @@ | |||
ul.pf-c-list:not(.pf-m-inline).co-detail-property-list { | |||
list-style-type: none; | |||
list-style: none; |
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.
don't these two result in the similar thing? Maybe we can use just list-style
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.
Right, removing.
list-style-type: none; | ||
list-style: none; | ||
padding-left: 0; | ||
li + li { |
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 do we need this?
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 to eliminate margin between li
s defined in .pf-c-list li+li
rule.
import { ListItem } from '@patternfly/react-core'; | ||
|
||
type Props = { | ||
title?: 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.
+ 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.
children
is already included in React.FC via React.PropsWithChildren type (https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/react/index.d.ts#L518)
<li>NICs: {ips}</li> | ||
<li>Boot Interface MAC: {getHostBootMACAddress(host)}</li> | ||
</ul> | ||
<DetailPropertyList> |
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 easier to use
6ed9924
to
09ff7d0
Compare
/retest |
1 similar comment
/retest |
/retest |
5 similar comments
/retest |
/retest |
/retest |
/retest |
/retest |
09ff7d0
to
4676587
Compare
/retest |
1 similar comment
/retest |
4676587
to
993b970
Compare
|
||
import './DetailPropertyList.scss'; | ||
|
||
const DetailPropertyList: React.FC<React.ComponentProps<typeof List>> = ({ |
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 use ListProps
directly
const DetailPropertyList: React.FC<React.ComponentProps<typeof List>> = ({ | |
const DetailPropertyList: React.FC<ListProps> = ({ |
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
|
||
const DetailPropertyListItem: React.FC<Props> = ({ title, children }) => ( | ||
<ListItem> | ||
{title && <span className="co-detail-property-list__item-title">{title}: </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.
missing import './DetailPropertyList.scss';
import. There would be an issue with missing styles if we use DetailPropertyListItem
without DetailPropertyList
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
import * as React from 'react'; | ||
import { ListItem } from '@patternfly/react-core'; | ||
|
||
type Props = { |
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 name this better - such as DetailPropertyListItemProps
and move to bottom as we usually do for type declarations ?
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've renamed the props but I'd prefer to keep them on top. Both conventions are used in console and I find putting props to the top more convenient as it's not necessary to scroll over the component body to see the props.
This adds common component for lists in detail page properties. - Common styling - Semantic use of lists instead of spans and brs
993b970
to
f36bf27
Compare
/retest |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jtomasek, rawagner, suomiy 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 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. |
This adds common component for lists in detail page properties.