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

React-ify scheduling check output tables #1721

Merged
merged 12 commits into from
Jul 31, 2015
69 changes: 66 additions & 3 deletions esp/esp/program/modules/handlers/schedulingcheckmodule.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def scheduling_checks(self, request, tl, one, two, module, extra, prog):
class Meta:
proxy = True

#For formatting output. The default is to use HTMLSCFormatter, but someone writing a script
#For formatting output. The default is to use JSONFormatter, but someone writing a script
#may want to use RawSCFormatter to get the original data structures
class RawSCFormatter:
def format_table(self, l, options={}, help_text=""):
Expand All @@ -47,7 +47,7 @@ def format_table(self, l, options={}, help_text=""):
def format_list(self, l, options={}, help_text=""):
return l

class HTMLSCFormatter:
class HTMLSCFormatter: # This might be dead, but idk maybe someone still wants it
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 it's specific enough to this context, and simple enough, that you can just nuke it. If anyone else wants it they can just grab it from git.

#requires: d, a two level dictionary where the the first set of
# keys are the headings expected on the side of the table, and
# the second set are the headings expected on the top of the table
Expand Down Expand Up @@ -109,9 +109,72 @@ def _format_dict_table(self, d, headings, help_text=""):
output += "</table>"
return output

# Builds JSON output for an object with attributes help_text, headings, and body.
class JSONFormatter:
#requires: d, a two level dictionary where the the first set of
# keys are the headings expected on the side of the table, and
# the second set are the headings expected on the top of the table
def format_table(self, d, options={}, help_text=""):
if isinstance(d, list):
return json.dumps(self._format_list_table(d, options['headings'], help_text=help_text))
else:
return json.dumps(self._format_dict_table(d, options['headings'], help_text=help_text))

def format_list(self, l, help_text=""): # needs verify
output = {}
output["help_text"] = help_text
output["headings"] = [] # no headings

# might be redundant, but it makes sure things aren't in a weird format
body = []
for row in l:
body.append(self._table_row([row]))
Copy link
Contributor

Choose a reason for hiding this comment

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

May be clearer to just do output["body"] = [self._table_row([row]) for row in l] and likewise in _format_list_table, _table_headings and other places.

output["body"] = body
return json.dumps(output)

def _table_headings(self, headings): # in case headings are a dict
Copy link
Contributor

Choose a reason for hiding this comment

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

I would just remove this function: at this point it's literally map(str,headings) which you can just inline.

#column headings
next_row = []
for h in headings:
next_row.append(str(h))
return next_row

def _table_row(self, row):
next_row = []
for r in row:
#displaying lists is sometimes borked. This makes it not borked
if isinstance(r, list):
r = [str(i) for i in r]
next_row.append(str(r))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to result in things having extra quotes, around them, no? (str(["a"]) == "['a']")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Empirically, no (I think e.g. lunch blocks is an example where you would expect that to happen). I think it's because javascript is rendering each cell separately.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it does happen.

['Sat Nov 22: 12:05 to 12:55 PM', 'Sat Nov 22: 1:05 to 1:55 PM']
['Sun Nov 23: 12:05 to 12:55 PM', 'Sun Nov 23: 1:05 to 1:55 PM']

But I guess that's existing behavior, and the alternative could be confusing, so it's fine.

return next_row

def _format_list_table(self, d, headings, help_text=""): #needs verify
output = {}
output["help_text"] = help_text
output["headings"] = self._table_headings(headings)
body = []
for row in d:
ordered_row = [row[h] for h in headings]
body.append(self._table_row(ordered_row)) # maybe?
output["body"] = body
return output

def _format_dict_table(self, d, headings, help_text=""): #needs verify
headings = [""] + headings[:]
output = {}
output["help_text"] = help_text
output["headings"] = self._table_headings(headings)

body = []
for key, row in sorted(d.iteritems()):
ordered_row = [row[h] for h in headings if h]
body.append(self._table_row([key] + ordered_row)) #maybe
output["body"] = body
return output

class SchedulingCheckRunner:
#Generate html report and generate text report functions?lingCheckRunner:
def __init__(self, program, formatter=HTMLSCFormatter()):
def __init__(self, program, formatter=JSONFormatter()):
"""
high_school_only and lunch should be lists of indeces of timeslots for the high school
only block and for lunch respectively
Expand Down
94 changes: 93 additions & 1 deletion esp/public/media/scripts/program/modules/scheduling_checks.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -80,11 +80,29 @@ var SchedulingCheck = React.createClass({
} else if (!this.state.data) {
body = <div className="placeholder">loading...</div>;
} else {
var data = JSON.parse(this.state.data); // Might not work on old browsers
var table;
if (data.headings.length == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This case and the other two I can find in this diff will be okay, but it's safer to more or less always use === instead of == in JS. (See the == equality table.)

var settings = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd just inline this in the <SelectTable>, I think that makes it easier to see what's happening. I'd also recommend just passing a single header attribute, instead of a settings object, since there is just one setting. If the number later becomes unmanageable, you can cross that bridge then.

header: false
};
table = <SelectTable rows = {data.body} settings = {settings} />;
} else {
var columns = [];
for (i = 0; i < data.headings.length; i++) {
if (!!data.headings[i]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the double-negation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, was trying to cast to boolean, but I guess it already did that.

columns[i] = {key: String(i), label: data.headings[i]};
} else {
columns[i] = {key: String(i), label: " "};
}
}
table = <SelectTable rows = {data.body} columns = {columns} />;
}
body = <div>
<div className="placeholder">
(loaded {this.state.timestamp}, click title to close)
</div>
<div className="data" dangerouslySetInnerHTML={{__html: this.state.data}} />
{table}
</div>;
}

Expand Down Expand Up @@ -114,3 +132,77 @@ var RefreshButton = React.createClass({
</button>;
},
});


// Modified from react-json-table example code.
var SelectTable = React.createClass({
getInitialState: function(){
// We will store the sorted column and whether each row is greyed out
var temp = new Array();
for (i = 0; i < this.props.rows.length; i++) {
temp[this.props.rows[i]] = false;
}
if (this.props.settings == undefined) {
this.props.settings = {
header: true
};
}
return {sort: -1, greyed : temp};
},
render: function(){
var me = this,
// clone the rows
items = this.props.rows.slice()
;
// Sort the table
if( this.state.sort ){
Copy link
Contributor

Choose a reason for hiding this comment

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

_.sortBy will do this in one line.

items.sort( function( a, b ){
return a[ me.state.sort ] > b[ me.state.sort ] ? 1 : -1;
});
}

if (this.props.columns == undefined) {
return <JsonTable
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than writing out the whole thing twice, it looks like if you just pass the undefined through, things should work correctly.

rows={items}
settings={ this.getSettings() }
onClickHeader={ this.onClickHeader }
onClickRow={ this.onClickRow }
/>;
} else {
return <JsonTable
rows={items}
columns={this.props.columns}
settings={ this.getSettings() }
onClickHeader={ this.onClickHeader }
onClickRow={ this.onClickRow }
/>;
}
},

getSettings: function(){
var me = this;
// We will add some classes to the selected rows and cells
return {
headerClass: function( current, key ){
if( me.state.sort == key )
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 a fan of explicit else clauses, but we don't have a style guide so you can ignore me if you want.

return current + ' headerSelected';
return current;
},
rowClass: function( current, item ){
if( me.state.greyed[item] )
return current + ' rowGreyed';
return current;
},
header: this.props.settings.header
};
},

onClickHeader: function( e, column ){
Copy link
Contributor

Choose a reason for hiding this comment

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

These two handlers are where you want to pass the state updates upwards, not in render.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did that initially; it didn't work. The parent gets passed an outdated version of the state (in particular, the most recent state change isn't processed). There's a thing on https://facebook.github.io/react/docs/component-api.html which seems to be relevant, about how it doesn't immediately mutate the state.

Copy link
Contributor

Choose a reason for hiding this comment

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

So I think what you want to do is to have all of the state exist in the SchedulingCheck, and none in the SelectTable -- SchedulingCheck can just pass what rows are grayed and such down to SelectTable as a prop. Then the event handler here just calls the callback from SchedulingCheck to update its state, which will then rerender a new SelectTable with a different set of rows grayed. (In fact, React is smart and will do the minimal set of DOM updates necessary to accomplish this.)

Either that or I don't understand the issue -- you are setting the new state to whatever you want it to be; React may batch updates, but if you make a change that change will get there eventually.

this.setState( {sort: column} );
},

onClickRow: function( e, item ){
this.state.greyed[item] = !this.state.greyed[item];
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 this is not a recommended approach, although it might work, it's better to actually put the change in the setState call.

this.setState(); // so that it actually updates
}
});
1 change: 1 addition & 0 deletions esp/templates/elements/html
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@

<script type="text/javascript" src="https://cdnjs.cloudflare.com/ajax/libs/react/0.13.3/react.js"></script>
<script type="text/javascript" src="https://cdnjs.cloudflare.com/ajax/libs/react/0.13.3/JSXTransformer.js"></script>
<script type="text/javascript" src="https://rawgit.com/arqex/react-json-table/master/build/react-json-table.min.js"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is only used on one page, I'd just put it there. Also, I would include a specific commit rather than master, just in case there is at some point a breaking change.

<script type="text/javascript" src="/media/scripts/lodash.compat.min.js"></script>

{% block js1 %}
Expand Down