feat(Network): add description field #848
Conversation
jamaljsr
force-pushed
the
network-description
branch
from
5bb8e95
to
9fe6fca
Compare
There was a problem hiding this comment.
Thanks for adding this new feature. I appreciate the contribution. I have some initial feedback.
I noticed that there is no way to modify a description after you initially create the network. I would think that this is something that users would want to do. I think a good place to do this is in the form displayed when you rename the node name. Add a field for the description here.
| @@ -181,12 +181,14 @@ export const testRepoState: DockerRepoState = { | |||
| export const getNetwork = ( | |||
| networkId = 1, | |||
| name?: string, | |||
| description?: string, | |||
There was a problem hiding this comment.
I would suggest moving this to be the last parameter of this method so that you do not have to update all of the calls to this method in the many tests.
Another approach is to just remove this parameter here and only set the description in the tests that need to update it. Ex:
const network = getNetwork(1, 'test network', Status.Started, 2);
network.description = 'my description';
| <Row> | ||
| <Styled.title>{`Description`}</Styled.title> | ||
| <Styled.description>{network.description}</Styled.description> | ||
| </Row> |
There was a problem hiding this comment.
It doesn't look good for networks without a description set. I also don't like the height of the card varying based on how long the description is. I would suggest using an "info" icon with a tooltip next to the name, and only display it if a description is defined.
| @@ -18,6 +18,7 @@ import { PolarPlatform } from 'utils/system'; | |||
| export interface Network { | |||
| id: number; | |||
| name: string; | |||
| description: string; | |||
There was a problem hiding this comment.
For users with existing networks, this value will be undefined when Polar starts and loads the data from networks.json. We do not want there to ever be a mismatch between the types and the actual runtime values. This can cause some pretty hard to find bugs.
I would suggest checking if this description is undefined at startup. If so, it should be set to an empty string so that the type is always correct.
| @@ -172,6 +172,7 @@ const NetworkView: React.FC<RouteComponentProps<MatchParams>> = ({ match }) => { | |||
| <Styled.PageHeader | |||
| colors={theme.pageHeader} | |||
| title={network.name} | |||
| description={'network.description'} | |||
There was a problem hiding this comment.
This is causing a Typescript error because there is no description prop on the PageHeader component.
|
Hey @Nonnyjoe do you plan on continuing to work on this PR? |
Closes #836
Description
Adds a description field to each network. On network creation users get to input the name and description of the network they intend to create, then while displaying a users network each card has a description component that displays the description of each network.
Screenshots