Skip to content

Add LineChart Component#30

Open
yunseop-kim wants to merge 3 commits into
AnSavvides:masterfrom
yunseop-kim:master
Open

Add LineChart Component#30
yunseop-kim wants to merge 3 commits into
AnSavvides:masterfrom
yunseop-kim:master

Conversation

@yunseop-kim

Copy link
Copy Markdown

Hello, Thanks for providing the good open source for us.

I added a line chart component, and it would be a pleasure for me if you review this source and merge it.

Have a good day!

@AnSavvides AnSavvides left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thanks for taking the time to do this, @yunseop-kim! I left a few comments for bits and pieces you can remove to make LineChart.js a little cleaner and without any debugging code like console.log.

It would also be wonderful if you could add a couple of examples! Check out the /examples/ folder and add an example in app.jsx - nice and quick way to showcase how this new line chart works and looks!

Comment thread src/components/LineChart.js Outdated
@@ -0,0 +1,152 @@
import * as d3 from "d3";
import BaseChart from './BaseChart';
// import {line} from 'd3-shape';

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Let's remove this :)

Comment thread src/components/LineChart.js Outdated
// import {line} from 'd3-shape';

export default class LineChart extends BaseChart {
getScaleX() { // *

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The // * can go :)

Comment thread src/components/LineChart.js Outdated
return d3.scaleTime().range([0, this.props.width]);
}

getScaleY() { // *

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The // * can go :)

Comment thread src/components/LineChart.js Outdated
return d3.scaleLinear().range([this.props.height, 0]);
}

createAxisX(x) { // *

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The // * can go :)

Comment thread src/components/LineChart.js Outdated
return d3.axisBottom(x);
}

createAxisY(y) { // *

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The // * can go :)

Comment thread src/components/LineChart.js Outdated
const y = this.getScaleY();

this.xAxis = this.createAxisX(x);//
this.yAxis = this.createAxisY(y);//

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Let's remove the // from these two lines.

Comment thread src/components/LineChart.js Outdated
d.y = +d.y;
});

console.log("data:", data);

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Forgot a console.log here :)

@yunseop-kim

Copy link
Copy Markdown
Author

@AnSavvides I removed unnecessary comments and added LineChart Example! Please review the source. :D

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