Dashboard for ArangoLocalStorage operator #213
Conversation
ghost
added
the
dashboard/src/api/api.js
Outdated
| @@ -1,13 +1,22 @@ | |||
| export function IsUnauthorized(e) { | |||
There was a problem hiding this comment.
FYI in most JS styles only classes and components use leading uppercase. So this should be isUnauthorized.
dashboard/src/App.js
Outdated
| const podInfoView = (<PodInfoView pod={pod} namespace={namespace}/>); | ||
| if (deployment) { | ||
| return (<DeploymentOperator podInfoView={podInfoView} error={error}/>); | ||
| } | ||
| if (storage) { | ||
| return (<StorageOperator podInfoView={podInfoView} error={error}/>); | ||
| } |
There was a problem hiding this comment.
If you want to avoid repeating yourself a bit, you could do something like this:
let Operator = NoOperator;
if (deployment) Operator = DeploymentOperator;
else if (storage) Operator = StorageOperator;
return (
<Operator
podInfoView={
<PodInfoView pod={pod} namespace={namespace} />
}
error={error}
/>
);| } | ||
| return (<LoadingView/>); | ||
| } | ||
| } | ||
|
|
||
| export default ReactTimeout(App); | ||
| export default ReactTimeout(withAuth(App)); |
There was a problem hiding this comment.
FYI this is why HOCs have been losing some traction to render props recently: you're now injecting props from two places. This is probably fine for the time being but it can get a bit confusing as the complexity grows.
| {this.props.podInfoView} | ||
| {(this.props.error) ? <Message error content={this.props.error}/> : null} | ||
| </div> | ||
| <Container> |
There was a problem hiding this comment.
This NoOperator component is exactly what I meant when I talked about presentational components btw. It doesn't have any state and it only contains display logic (i.e. "if there's an error message, put it here"). If you want to make it easier to swap out the styling later, you should try to extract all the actual presentation stuff into components like this one.
Minor nitpick: no point in using a class component here, you could just make it a function.
| @@ -12,6 +13,7 @@ const LoginView = ({username, password, onUsernameChanged, onPasswordChanged, do | |||
| <label>Password</label> | |||
| <input type="password" value={password} onChange={(e) => onPasswordChanged(e.target.value)}/> | |||
| </Form.Field> | |||
| <Form.Button className={css`display:none`} type="submit" /> | |||
There was a problem hiding this comment.
Any particular reason you have a hidden submit button here? Pressing enter in the input field should already trigger the form submit.
There was a problem hiding this comment.
Also curious why you're using css here but styled elsewhere as most uses of styled can also be replaced with css (and generally vice versa unless you want to apply the same class to different components, in which case you'll always need css).
There was a problem hiding this comment.
hidden button was out of need. I expected it to work without, but it did not.
There was a problem hiding this comment.
I like styled better, but that did not work in combination with Form.Button.
| return (<ListView items={items} loading={this.state.loading} />); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
This file is getting rather large. I'd split it out to a folder. I generally aim to have one component per folder but YMMV.
There was a problem hiding this comment.
once all the basic are in, I'll go over all of it for issues like this.
This PR adds dashboard functionality to the ArangoLocalStorage operator.