Added main workshop dashboard UI

[aoby]

To make this a little smaller, I removed the reports from this first set of changes. The reports aren't needed for facilitator summit so I can add them back in and review next week.

cc @islemaster @pcardune @breville

Note: some of the css is still being worked out.

Screenshots:

New:

Workshop index:

View (not started):

followed by:

Clicking edit opens the fields for edit.

View (in progress):

Attendance

[pcardune]

anything that gets used in the front end should probably be part of the dependencies section. I don't think it will make a difference the way we have things set up, but "best practices" and all that.

[Bjvanminnen]

idk, dependencies are what's required to run, devDependencies are required to develop. In this case, these packages are used to create a build product, but none of them are required to consume the resulting package. I think this is the argument we've been using for putting everything in devDependencies.

[Bjvanminnen]

Might be interesting to see what this all does for package size. I know that for moment at least, I've used it in personal projects until realizing that it's footprint was a lot bigger than justified for how I was using it.

[aoby]

Oh yeah. It certainly is convenient though. Maybe we can find a lighter-weight alternative, or just do the parsing and formatting ourselves (though that's obviously less desirable).

[Bjvanminnen]

Yeah, it's a pretty nice library. Presumably we're only requiring it in this new bundle so far too, which means we're not actually growing anything?

[aoby]

Right, "so far". But it might set a precedent and encourage others ;)

[Bjvanminnen]

Metapoint: A lot of my comments are things that don't necessarily have to be addressed immediately (i.e. shouldn't block checkin). I'll try to prepend these with "nit" as much as I can. These are things that may be worth looking at/thinking about, but don't let them prevent you from merging.

[Bjvanminnen]

Same story with modifying state, this should just be this.setState({adminOverride: !this.state.adminOverride})

[Bjvanminnen]

Directly modifying this.state

[islemaster]

Wow! Only four lines removed?

[Bjvanminnen]

nit: Just realized you've been using snake_case for your component files. Our preference elsewhere has been for CamelCase, though it's probably not worth going through and changing everything

[aoby]

It's actually inconsistent. See https://github.com/code-dot-org/code-dot-org/tree/staging/code-studio/src/js/components (send_to_phone.jsx, share_dialog.jsx, etc). But those are in the minority. I guess this is just ruby style creep on my part. I'll update this one after the initial commit.

[Bjvanminnen]

You are right that we haven't been entirely consistent (I'm the guilty party that created those components). Not that big a deal if these are different if changing will be a lot of work.

[Bjvanminnen]

Overall, this looks really good. I had a decent number of style nits, but really there were a lot fewer than I expected there to be :)

I think the one big thing I'd like to see changed is that there are a bunch of places where you're directly modifying this.state which is a big no-no in React. Hopefully most of these are pretty easy to fix.

I'd also like to see a few more comments, especially at the component level summarizing what each component's purpose is.

My other big meta-point is that we'd really rather never be in a position where we're reviewing a huge PR like this on a deadline. It makes it so that it's more difficult to look at everything as closely, and it becomes riskier to actually make any changes based on feedback (since there's the pressure to get things committed). As a team, I think we should make an effort to have large features like this broken into a series of smaller commits, that can receive/iterate on feedback earlier rather than having one giant change.

Let me know if there's anything I can do to help, either testing changes or in some other way!

[joshlory]

Super small nit: these can be handleDataChange(e) { in ES6.

[Bjvanminnen]

Changes look good. Left a few comments on the commit, most of which can probably be ignored or put off if you want. The only one I'd be sure to take a look at was where you could end up modifying another component's props object.

[Bjvanminnen]

I think the cloning for sessions needs to happen sooner (i.e. before we mutate it), otherwise we're still changing the props of the caller.

I don't think we need to clone destroyedSessions at all as this is just a local object.

i.e. I would expect:

handleSessionsChange: function (sessions, removedSession) {
    sessions = _.cloneDeep(sessions); 
    sessions.sessionsModified = true;
     var destroyedSessions = [];
     if (removedSession && removedSession.id) {
        destroyedSessions.push(removedSession);
      }
      this.setState({
        sessions: sessions,
        destroyedSessions: destroyedSessions
      });

At this point if you want you could take advantage of an ES6 feature and simplify the last bit to this.setState({sessions, destroyedSessions})

[Bjvanminnen]

lgtm