Skip to content

cmd, dashboard: dashboard using React, Material-UI, Recharts#15393

Merged
karalabe merged 5 commits into
ethereum:masterfrom
kurkomisi:dashboard_react_mui_recharts
Nov 14, 2017
Merged

cmd, dashboard: dashboard using React, Material-UI, Recharts#15393
karalabe merged 5 commits into
ethereum:masterfrom
kurkomisi:dashboard_react_mui_recharts

Conversation

@kurkomisi
Copy link
Copy Markdown
Contributor

Work in progress dashboard prototype.
Description in the README file.

Comment thread .gitignore Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix all your files so they have a newline at the end.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also skip the bundle.js, since it's an intermediate build step between source jsx and the final assets.go

Comment thread cmd/utils/flags.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls drop "default"

Comment thread dashboard/README.md Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for double title.

Comment thread dashboard/README.md Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for user docs, that may or may not be done in some global documentation.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--dashboard should auto-enable --metrics.

Comment thread dashboard/assets/components/Common.jsx Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless really really often needed, please just inline the actual code instead of the defaultZero (avoids problems with non-int types).

Comment thread dashboard/assets/components/Common.jsx Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets use the following order:

  • Home
  • Chain
  • Transactions
  • Network
  • System
  • Logs

@karalabe
Copy link
Copy Markdown
Member

Please add the LGPL3 copyright headers to all (non-generated) files

// Copyright 2017 The go-ethereum Authors
// This file is part of the go-ethereum library.
//
// The go-ethereum library is free software: you can redistribute it and/or modify
// it under the terms of the GNU Lesser General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.
//
// The go-ethereum library is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU Lesser General Public License for more details.
//
// You should have received a copy of the GNU Lesser General Public License
// along with the go-ethereum library. If not, see <http://www.gnu.org/licenses/>.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use theme ;)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls align these a bit "nicer" ;P

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible, please align struct initializers. An automated tool is even better, a CI linter is possibly the best.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new WebSocket(((window.location.protocol === "https:") ? "wss://" : "ws://") + window.location.host + "/api");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhas call metrics -> history.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please retain data arrays here and only convert later when actually rendering. Also I don't think there's a need to have specialized memory and traffic datakeys, we can just use value as the key (my 2c, open for debate).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Align pls.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No :P Pls expand

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls use TRAFFIC_SAMPLE_LIMIT for the traffic if you defined it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also a bit more flexible would be to do

while (memory.length >= MEMORY_SAMPLE_LIMIT) {
    memory.shift();
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • while
  • const
  • 4096

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if

Comment thread dashboard/assets/components/Main.jsx Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use theme colors everywhere instead of custom ones so they can be easily swapped out with the theme.

Comment thread dashboard/assets/components/SideBar.jsx Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

load = event => {
    event.preventDefault();
    this.props.changeContent(event.target.id);
};

Comment thread dashboard/assets/components/SideBar.jsx Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

id={tag.id} onClick={this.load} ...

Comment thread dashboard/dashboard.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before go-bindata, I'd suggest also regenerating the bundle.js to a dev doesn't need to remembed the "correct" workflow, rather just use go generate and be done with it.

Comment thread dashboard/dashboard.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls drop dead code.

Comment thread dashboard/dashboard.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls drop dead code.

Comment thread dashboard/dashboard.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets rename this to chartEntry

Comment thread dashboard/dashboard.go Outdated
Copy link
Copy Markdown
Member

@karalabe karalabe Nov 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets rename metricSamples to charts, containing []chartEntry

Comment thread dashboard/dashboard.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please inline this and group by chart type, not by operation.

Comment thread dashboard/dashboard.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

filepath.Join :)

Comment thread dashboard/dashboard.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new(charts)

Comment thread dashboard/dashboard.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps rename Metrics to Charts

Comment thread dashboard/dashboard.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quit is usually the thing used in the go-ethereum codebase

Comment thread dashboard/dashboard.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't ignore errors, log at least, preferrably store and return at the end of the function.

Comment thread dashboard/dashboard.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

webapp -> blob.

Comment thread dashboard/dashboard.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file -> blob

Comment thread dashboard/dashboard.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls add path to the error log.

Comment thread dashboard/dashboard.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done := make(chan struct{})

Comment thread dashboard/dashboard.go Outdated
Copy link
Copy Markdown
Member

@karalabe karalabe Nov 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

close(done)

Comment thread dashboard/dashboard.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need a pointer, map is already a lightweight wrapper.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also please convert the map[string]interface{} to a proper type with typed fields and json tags.

Comment thread dashboard/dashboard.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(drop)

Comment thread dashboard/dashboard.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc sendToAll

@kurkomisi kurkomisi force-pushed the dashboard_react_mui_recharts branch from a172ac2 to c48c55f Compare November 14, 2017 13:45
@karalabe karalabe force-pushed the dashboard_react_mui_recharts branch from 7a34abc to a77f486 Compare November 14, 2017 17:04
Copy link
Copy Markdown
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@karalabe karalabe merged commit ba62215 into ethereum:master Nov 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants