Skip to content
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

[WIP] Activity chart #14

Merged
merged 34 commits into from Feb 6, 2017
Merged

[WIP] Activity chart #14

merged 34 commits into from Feb 6, 2017

Conversation

flowirtz
Copy link
Contributor

@flowirtz flowirtz commented Jan 7, 2017

Activity chart diagram in the style of GitHub activity

Todos

  • don't hardcode data
  • more (@jak-ing knows)

MaximilianV and others added 2 commits January 8, 2017 12:13
maybe we have an js expert who can find a cleaner approach to solve the "this" problem.
…rect is set relative to the label's text length.
Copy link
Contributor Author

@flowirtz flowirtz left a comment

Choose a reason for hiding this comment

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

Really like the changes, makes the code way cleaner.

@@ -106,29 +106,57 @@
},

_showLabel: function (d, i) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

even though this method is private, we might wanna use more meaningful names than d, i

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm against that here. It's a D3 thing to use precisely this signature; d being the datum and i being its index.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, I see. Sorry for the confusion!

//append rect element first so that following text element is displayed above rect
label.append('rect');

var labelText = label.append('text')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

like this approach

//finish rect configuration that was appended previously
label.select('rect')
.attr('x', labelText.attr('x') - 3)
.attr('y', labelText.attr('y') - 11)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

where do all these hardcoded values (y -11 etc., rather e.g. y - something.width) come from and are they needed like this (probably not)?
Can you maybe rather put them into a var as well and calc them respectively?

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed in e4b408e or the necessity of hard coded values is explained thoroughly. ;)

d3.select('#label-' + i).remove();
},

_spaceLabelHor: function(d, i, labelWidth) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe rename d, i to something more meaningful

if dataurl is provided, this will be used, otherwise the parameters dataset and labels are processed.
@@ -72,27 +82,35 @@
* @param {Object} data The data to be plotted
*/
handleResponse: function (data) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe make that private?

//so far, the appropriate color is calculated based on the value of the datum, there is no set of fixed colors
var colorScale = d3.scaleLinear()
.domain([datasetMin, datasetMax])
.range(['#F5E8BB', '#930517']);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

make that a parameter 😊 maybe

MaximilianV and others added 9 commits January 11, 2017 10:47
replacing ready by attached has done the trick
Set SVG height and width dynamically based on the size of the dataset and squaresPerRow. Use ES6 notation for calculating min and max values of the dataset.
Make the dataset that is passed to D3 and which the visualization is built from look like this: [ {'x': timeStamp0, 'y': value0}, ... ,{'x': timeStampN, 'y': valueN}]
Use the two custom colors marking the minimum and the maximum color. Custom colors can be passed to the component via an attribute in the HTML tag.
Change orientation changing the focus from rows to columns.
Improve the calculation of positions and sizes for the labels to make these values more consistent among different square sizes, spacing options etc by mostly dropping hardcoded values.
Allow the minimum and maximum value used for the color scale to be overwritten by arguments in the HTML tag. By default, these values are derived from the dataset.
Copy link
Contributor Author

@flowirtz flowirtz left a comment

Choose a reason for hiding this comment

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

LGTM when finished

@@ -13,6 +13,9 @@
<link rel="import" href="../activitychart-basic.html">

<style is="custom-style" include="demo-pages-shared-styles">
div.vertical-section-container {
max-width: 1500px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems to not be working on Mac w/ Firefox though, couldnt find a reason why... see #25

on-response="handleResponse"
debounce-duration="300">
</iron-ajax>
<div style="display:none;">{{dataset}}</div>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

that bloody issue...
see #26

Copy link
Contributor Author

@flowirtz flowirtz left a comment

Choose a reason for hiding this comment

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

Looks great!
Do we want to remove the dataurl and AJAX part and everything related to it though, so we can put that into another component effectively letting this one here only accept direct json via the data attributes, as discussed? see #28

return data;
},

_plotDiagram: function () {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please document

this._plotDiagram();
},

_buildDataset: function(labels, dataset) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please document

_showLabel: function (d, i) {
//using a group element 'label' with an appended text element and a rect element as background to display a label

var polyElement = Polymer.dom(this).parentNode.parentNode.parentNode;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is there maybe a better way to traverse the dom than .parentNode.parentNode.parentNode? Seems kinda odd to me, definitely don't know better though... 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sure there is; Jan made a remark about this earlier.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's discuss / investigate in the next team meeting

},

attached: function() {
if(this.dataurl != "") {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the != "" part can be omitted afaik
http://stackoverflow.com/a/154068


var data = this._buildDataset(this.labels, this.dataset);

var dataset = this.dataset;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if we move this line above var data = this._buildDataset(this.labels, this.dataset); we can omit the this. in the latter part of the params

//append rect element first so that following text element is displayed above rect
label.append('rect');
var labelText = label.append('text')
.text(d.y + ' at ' + d.x)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is the at part hardcoded? Might be bad for translations

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, changed this in cf43575.

if (Polymer.dom(this).node.nodeName != "ACTIVITYCHART-BASIC") {
var polyElement = Polymer.dom(this).parentNode.parentNode.parentNode;
}
return polyElement.squareSpacing * (Math.floor(i / polyElement.squaresPerColumn));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe we should use a pre-commit hook for beautify.js

JE and others added 4 commits January 19, 2017 17:30
Omitted unnecessary != "" part in if
Moved a var declaration above its use
SquaresPerColumn is now always derived from the number of axis labels. Add stub for alternating between user and course activity. Fix Polymer.dom(this).parentNode problem. Limit D3 to only scan the Polymer component's local DOM instead of the whole DOM thus avoiding problems when instantiating a component more than once on a page.
@Langleu
Copy link
Contributor

Langleu commented Feb 6, 2017

👍

Copy link
Contributor

@JakobEdding JakobEdding left a comment

Choose a reason for hiding this comment

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

Appears to be ready to be merged into master.

@JakobEdding
Copy link
Contributor

👍

@Langleu Langleu merged commit edc6071 into master Feb 6, 2017
@Langleu Langleu deleted the activity-chart branch February 6, 2017 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants