Skip to content
This repository has been archived by the owner on Feb 22, 2024. It is now read-only.

Commits over time chart #486

Merged
merged 23 commits into from
Jul 11, 2017
Merged

Commits over time chart #486

merged 23 commits into from
Jul 11, 2017

Conversation

herfstregen
Copy link
Contributor

No description provided.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 56.636% when pulling 41ef5d7 on commitGraph into 29e9043 on master.

@asylunatic asylunatic requested review from jwgmeligmeyling and removed request for jwgmeligmeyling June 28, 2017 11:04

<div class="row">
<div class="col-md-10 col-md-offset-2">
<h4 style="line-height:34px; margin-top:0;">Nice graphs &nbsp; 😍</h4>
Copy link
Member

Choose a reason for hiding this comment

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

Can you please replace all "Nice graphs" occurrences with a property within our localisation file? (so ${i18n.translate("header.graphs")} and then in devhub_en.properties you add: header.graphs = .... We may reconsider to change the title too. I would keep the emoji.

Copy link
Contributor

Choose a reason for hiding this comment

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

I did, but it killed the emoji :'(

return getGoogleChartDataForCommits(commitEntities);
}

private static List<List<Object>> getGoogleChartDataForCommits(List<Commit> commitEntities) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any mention or communication with Google in this method. Should it be called getGoogleChartDataForCommits?

Copy link
Member

Choose a reason for hiding this comment

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

It doesn't call Google but it represents a data format specifically for Google Charts. So the naming is fine.

vAxis: {minValue: 0, gridlines: {count : -1}}
};

var mijnMagischeElement = $('<div>')
Copy link
Contributor

Choose a reason for hiding this comment

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

Dutch?

Copy link
Contributor

Choose a reason for hiding this comment

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

¯_(ツ)_/¯ maybe to make the magic even more magic? :')


google.charts.load('current', {'packages':['corechart']});
google.charts.setOnLoadCallback(function() {
$.get('http://localhost:50001/courses/ti1705/TI1705/groups/1/person-commit')
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be the production url, or relative from the current url.

Copy link
Member

Choose a reason for hiding this comment

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

It should be relative indeed. (/courses...)

var randomcolor = getRandColor(4);
google.charts.load('current', {'packages':['corechart']});
google.charts.setOnLoadCallback(function() {
$.get('http://localhost:50001/courses/ti1705/TI1705/groups/1/magical-chart-data')
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be the production url, or relative from the current url.

Copy link
Member

Choose a reason for hiding this comment

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

It should be relative indeed. (/courses...)

Copy link
Member

Choose a reason for hiding this comment

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

And it should dynamically get the group and course numbers

Copy link
Member

Choose a reason for hiding this comment

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

Good catch 😉

<script type="text/javascript" src="http://code.stephenmorley.org/javascript/colour-handling-and-processing/Colour.js"></script>
<script type="text/javascript">

function getRandColor(brightness){
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you copy this from an external resource? Would be better to include a link to it, maybe StackOverflow?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I did yes, you want a link for reference?


<!-- Graph for all commits -->
<div class="col-md-10">
<div id="allcommits_div" style="width: 100%; height: 500px; "></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Include styling in a <style> instead, not in-line

Copy link
Member

Choose a reason for hiding this comment

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

Or in the sass files perhaps.

@TimvdLippe
Copy link
Contributor

TimvdLippe commented Jul 1, 2017 via email

Fastjur
Fastjur previously requested changes Jul 2, 2017
Copy link
Member

@Fastjur Fastjur left a comment

Choose a reason for hiding this comment

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

Please remove the localhost:5001 lines. Other than that everything that has not got comments looks good!


<!-- Graph for all commits -->
<div class="col-md-10">
<div id="allcommits_div" style="width: 100%; height: 500px; "></div>
Copy link
Member

Choose a reason for hiding this comment

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

Or in the sass files perhaps.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 56.85% when pulling fa7c6f2 on commitGraph into 29e9043 on master.


google.charts.load('current', {'packages':['corechart']});
google.charts.setOnLoadCallback(function() {
$.get('/courses/ti1705/TI1705/groups/1/person-commit')
Copy link
Contributor

Choose a reason for hiding this comment

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

Also this should dynamically load, haven't figured that out though

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 56.85% when pulling 4f52d38 on commitGraph into 29e9043 on master.

@jwgmeligmeyling jwgmeligmeyling dismissed Fastjur’s stale review July 10, 2017 22:16

I've pushed changes that make the path truly relative.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 56.6% when pulling 497c068 on commitGraph into 29e9043 on master.

@jwgmeligmeyling jwgmeligmeyling changed the title Commit graph Commits over time chart Jul 10, 2017
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 56.6% when pulling 3c126b0 on commitGraph into 29e9043 on master.

@jwgmeligmeyling jwgmeligmeyling merged commit c107c37 into master Jul 11, 2017
@jwgmeligmeyling jwgmeligmeyling deleted the commitGraph branch July 11, 2017 11:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants