React-ify scheduling check output tables #1721

jerrywu64 · 2015-07-02T22:03:49Z

Addresses #1687.

Some notes (some of which are mentioned in in-code comments):
I didn't touch HTMLSCFormatter. It looks to me like there isn't actually any code which uses it (except for scheduling checks obviously except those now use JSONFormatter), but idk if it'll be useful in the future.
JSONFormatter possibly contains redundant code, but maybe the code is useful for making sure things are formatted properly-- I didn't check preconditions/postconditions carefully enough to tell.
I'm using JSON.parse() which apparently isn't supported in old browsers, idk if we care about that for a management page.
Changes in public/media/styles/scheduling_checks.css are needed, but that's in .gitignore; stuff I added is below. (My devserver originally didn't even have this file, so I don't know what other people's scheduling_checks.css files look like or if this conflicts.)
Scheduling check tables now enable sorting (upon header click) and greying-out of a row (upon row click, provided that you make the appropriate change in scheduling_checks.css. I didn't add any buttons to this effect, though (in my css I did do cursor:pointer-- although it's for the whole table so this is kind of silly), and the sorting can be kinda sketchy.
Sorting and greying does not persist across closing and reopening the scheduling check, let alone refreshing.

Additions to scheduling_checks.css:
table { border-spacing: 0; }
td { padding: 10px; cursor: pointer; }
th { cursor: pointer; }
.rowGreyed {
color: grey;
}
.headerSelected {
color: blue;
}

People probably have opinions on the changes made, so feel free to comment. Although I note that if you want slightly different behavior upon clicking a row, if it's doable in css, you can probably just do it in css on your end.

benjaminjkraft · 2015-07-03T01:01:10Z

esp/esp/program/modules/handlers/schedulingcheckmodule.py

@@ -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


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.

…ing and the refresh button.

jerrywu64 · 2015-07-05T00:47:00Z

Sorting and greying should now persist across hide/unhide and the refresh button, but not refreshing the page itself. This includes if you press the refresh button and something changes.

Expected behavior is that if the data in a row changes, it reverts to the default state (e.g. not greyed if it was greyed before).

However, I also expect that if for whatever reason a scheduling check has two duplicate rows, it'll be impossible to apply different formatting to them, i.e. if you try to grey one, it'll also grey the other. (Although I haven't actually tested this.) Do people think this is an issue?

benjaminjkraft · 2015-07-07T01:24:02Z

esp/public/media/scripts/program/modules/scheduling_checks.jsx

+        var settings = {
+          header: false
+        };
+      table = <SelectTable rows = {data.body} settings = {settings} saveState = {this.state.tableState} />;


This should be indented one more.

benjaminjkraft · 2015-07-07T02:58:24Z

Cool! I don't particularly care about the exact CSS; I'd suggest maybe making the gray a little lighter, but I don't feel strongly. I left a bunch of inline comments on the code style, but I think it overall looks good.

I'm using JSON.parse() which apparently isn't supported in old browsers, idk if we care about that for a management page.

That's fine, I don't think there are many admins using IE 7.

Changes in public/media/styles/scheduling_checks.css are needed, but that's in .gitignore; stuff I added is below. (My devserver originally didn't even have this file, so I don't know what other people's scheduling_checks.css files look like or if this conflicts.)

You want to look at esp/public/media/default_styles/scheduling_checks.css, and edit that. Most dev servers just link esp/public/media/styles/ to that directory; some older setups have the directories actually copied.

However, I also expect that if for whatever reason a scheduling check has two duplicate rows, it'll be impossible to apply different formatting to them, i.e. if you try to grey one, it'll also grey the other. (Although I haven't actually tested this.) Do people think this is an issue?

I can't think of a reason this would happen (other than someone forgetting a .distinct() in a query), so I think it's fine.

mgersh · 2015-07-07T19:20:54Z

The reason why @jerrywu64's dev server has a weird media setup is because it's fabric/Vagrant on Windows, see https://github.com/learning-unlimited/ESP-Website/blob/main/esp/fabfile.py#L125

benjaminjkraft · 2015-07-07T19:24:32Z

Ah, ok. (Another reason we should get rid of this setup, but I digress.) In that case, yeah, just actually copy the directories.

benjaminjkraft · 2015-07-07T19:29:41Z

esp/public/media/scripts/program/modules/scheduling_checks.jsx

@@ -80,11 +93,32 @@ 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) {


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.)

…d empty headers with --

benjaminjkraft · 2015-07-07T22:32:46Z

esp/public/media/scripts/program/modules/scheduling_checks.jsx

+    // clone the rows
+    items = this.props.rows.slice();
+
+    items = _.sortBy(items, function( item ){


I believe you can actually do _.sortBy(items, me.state.sort) and it will automatically do the lookup.

jerrywu64 · 2015-07-07T23:03:08Z

Updates on css:

Something like
.reset-button {
margin-left: auto;
float: right;
}

If you're sorting a column in descending order, if you've added formatting for sorted columns it'll still use that, but you can also override the formatting, e.g.
.sortReversed {
color: red;
}
(although this will have to go after the formatting for .headerSelected).

Also, note that sorting currently doesn't "stack": yes, we're using a stable sort, but if you sort first by one column then by a second, it'll use the original ordering as tiebreaker for the second column instead of the first ordering.

This could conceivably be fixed by saving the current set of rows in state, but reverse sorting may produce unwanted behavior, e.g. if you sort by column 1 and then by column 2, it'll give the reverse of sort-by-2-using-1-as-tiebreaker, which has the opposite tiebreaker order from sort-descending-by-2-using-1-as-tiebreaker. Or maybe that's the behavior people want, and regardless it could be fixed by tracking the previous sorted column and not updating the stored rows until that changes.

benjaminjkraft · 2015-07-07T23:29:02Z

Can you put the css into esp/public/media/default_styles/scheduling_checks.css? If you then copy that into esp/public/media/styles locally it should work the way it will on an actual server.

For multicolumn sorting, I would just not worry about it for now.

benjaminjkraft · 2015-07-09T01:54:07Z

esp/public/media/scripts/program/modules/scheduling_checks.jsx

    };
  },
+
+  updateTableState: function (state) {


You want to use this.setState, not modify this.state directly.

benjaminjkraft · 2015-07-09T01:59:23Z

Thanks. I left a few more comments -- mostly just some issues with how you're modifying state. Sorry this is taking so many rounds -- I know React can be a bit tricky to get used to, but I promise most of the constraints exist for good reason.

benjaminjkraft · 2015-07-09T02:16:29Z

esp/public/media/scripts/program/modules/scheduling_checks.jsx

+        sort: -1,
+        reverse: false
+      },
+      open: false


I find it confusing that reset closes the box -- is there a reason for that?

I could possibly try to implement it so that it's unnecessary, but currently I have it so that rendering the child updates the parent's record of the child's state. Reset is supposed to force the child to be recreated, but if I don't close the box the child's state will "update" the parent's record and then we will be sad.

Read below -- I don't think having rendering the child update the parent's state is a good strategy anyway, and I don't think it's any harder to do things right.

…eck, fixed data flow issues

jerrywu64 · 2015-07-09T23:38:56Z

Let me know if we would rather use react than react-with-addons, I'm using react-with-addons for update(). Those portions can be rewritten to be done manually if needed, the objects I'm updating aren't very big.

benjaminjkraft · 2015-07-30T20:42:44Z

I don't have a particular problem with using react-with-addons. It might be easier all around to not have tableState be a separate object, and have it just be three more keys in state, in which case you wouldn't need the complicated update thing, but I don't particularly care, so feel free to leave it as-is.

benjaminjkraft · 2015-07-30T20:45:49Z

esp/public/media/scripts/program/modules/scheduling_checks.jsx

+  },
+
+  getInitialState: function(){
+    return {sort: this.props.saveState.sort, greyed: this.props.saveState.greyed, reverse: this.props.saveState.reverse};


This state is no longer getting modified, so you may as well just remove it, and change the references to get everything directly from props.

benjaminjkraft · 2015-07-30T20:46:40Z

In any case, I just left a few comments, but I think other than those couple of minor issues, this is good. Thanks for your patience with all my comments :-) .

jerrywu64 · 2015-07-30T22:13:44Z

Cool. I'm not going to bother changing tableState to be different, although yes the current setup doesn't quite make sense.

And nah, thanks for your help; I should really get used to using things more in line with how they're supposed to be used instead of hacking things together (and probably more generally figuring out how things are supposed to be used in the first place), and also checking over my code so I don't have the problem in which stuff is set up a certain way because of a previous implementation attempt that has since been scrapped oops.

benjaminjkraft · 2015-07-31T17:00:59Z

Cool! I think this looks good now, merging.

React-ify scheduling check output tables

jerrywu64 added 3 commits July 1, 2015 17:43

Converted scheduling checks HTML table to React table

c5694e3

Implemented greying upon row click, also sorts by column

13248a4

Removed some commented-out stuff

ecb6011

benjaminjkraft reviewed Jul 3, 2015
View reviewed changes

Removed HTMLSCFormatter, caused sorting/greying to persist across hid…

75f3c01

…ing and the refresh button.

benjaminjkraft reviewed Jul 7, 2015
View reviewed changes

jerrywu64 added 3 commits July 7, 2015 12:55

Some stylistic changes

9f5f43c

Restructured so that props are not mutated

82e87d7

Caused numbers to be sorted properly, added reverse sort, and replace…

0b18d5f

…d empty headers with --

benjaminjkraft reviewed Jul 7, 2015
View reviewed changes

Added reset button

b7f2ba2

Updated default_styles css

af60cfa

benjaminjkraft reviewed Jul 9, 2015
View reviewed changes

Changed react to react-with-addons, made reset no longer close the ch…

0bc91b6

…eck, fixed data flow issues

benjaminjkraft reviewed Jul 30, 2015
View reviewed changes

Removed SelectTable state and changed references to props

eeb1a74

removed tableKey

4647488

benjaminjkraft added a commit that referenced this pull request Jul 31, 2015

Merge pull request #1721 from learning-unlimited/hide-scheduling-checks

5e11257

React-ify scheduling check output tables

benjaminjkraft merged commit 5e11257 into main Jul 31, 2015

benjaminjkraft deleted the hide-scheduling-checks branch July 31, 2015 17:01

benjaminjkraft mentioned this pull request Mar 10, 2016

Scheduling checks: a way to hide false positives #1687

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

React-ify scheduling check output tables #1721

React-ify scheduling check output tables #1721

jerrywu64 commented Jul 2, 2015

benjaminjkraft Jul 3, 2015

jerrywu64 commented Jul 5, 2015

benjaminjkraft Jul 7, 2015

benjaminjkraft commented Jul 7, 2015

mgersh commented Jul 7, 2015

benjaminjkraft commented Jul 7, 2015

benjaminjkraft Jul 7, 2015

benjaminjkraft Jul 7, 2015

jerrywu64 commented Jul 7, 2015

benjaminjkraft commented Jul 7, 2015

benjaminjkraft Jul 9, 2015

benjaminjkraft commented Jul 9, 2015

benjaminjkraft Jul 9, 2015

jerrywu64 Jul 9, 2015

benjaminjkraft Jul 9, 2015

jerrywu64 commented Jul 9, 2015

benjaminjkraft commented Jul 30, 2015

benjaminjkraft Jul 30, 2015

benjaminjkraft commented Jul 30, 2015

jerrywu64 commented Jul 30, 2015

benjaminjkraft commented Jul 31, 2015

React-ify scheduling check output tables #1721

React-ify scheduling check output tables #1721

Conversation

jerrywu64 commented Jul 2, 2015

Choose a reason for hiding this comment

jerrywu64 commented Jul 5, 2015

Choose a reason for hiding this comment

benjaminjkraft commented Jul 7, 2015

mgersh commented Jul 7, 2015

benjaminjkraft commented Jul 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerrywu64 commented Jul 7, 2015

benjaminjkraft commented Jul 7, 2015

Choose a reason for hiding this comment

benjaminjkraft commented Jul 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerrywu64 commented Jul 9, 2015

benjaminjkraft commented Jul 30, 2015

Choose a reason for hiding this comment

benjaminjkraft commented Jul 30, 2015

jerrywu64 commented Jul 30, 2015

benjaminjkraft commented Jul 31, 2015