Darien classrooms table #74

srallen · 2017-10-19T19:40:45Z

This will have to merge after #73 because it is using the program filter for classrooms, so please ignore the changes that pertain to #73 until this is rebased.

This adds a view for the Darien educators page which is basically the classrooms manager which toggles between the table list view or the classroom editor. Darien's table will be slightly different than I2A's:

the ability to create and edit assignments which I imagine will open a modal with a form like classroom create/edit does. Right now I just have placeholder buttons for creating and editing an assignment in the markup.
The actual data in the table for the assignments. I replaced the export data column for the due date.

I really wanted to try to be clever with determining which classroom table to display by trying out some higher order components, but after puzzling on this for a while I ended up settling with a ternary. The nest of components to get down to the classrooms table is a bit convoluted and I'd really like to try to clean this up at some point as I'm finding it hard to follow myself and I wrote most of it.

Part of the issue is React Router 4's new way of declarative routes, so there are a few components who's primary purpose is to contain the <Switch /> and <Routes /> for whatever nested routes they are.

The other issue is that Darien and I2A share all of the classroom components up until the table, so I couldn't figure out a clever way to have the routes programmatically determine the table since the table itself isn't going to be a route; it's just a child component of the <ClassroomsManagerContainer /> component that is assigned to the /educators/classrooms route. The I2A /educators also doesn't exist on its own; the I2A home page redirects to the educators page since they won't have a student interface.

If you have any good ideas on how to make this better, I'm definitely open to it.

srallen · 2017-10-19T19:41:56Z

Noting that this is for issue #70

shaunanoordin · 2017-10-20T15:09:35Z

I've done a quick review on the user side of things, and here are some observations - although much of what I say may be rendered moot when this PR is rebased.

Overall layout & presentation: 👍
- Also 👍 on the placeholders that will eventually open the Create Assignment modal; that's the path I'd like to go down.
- Thought: How the Create Assignment modal will be built, though, will need better planning, since Darien's Create Assignment will have a project-specific feature that allows it to communicate with the Map Explorer. (i.e. how can this be generalised?)
Quirks:
- [medium] I can create Classrooms in Darien, but the Classroom won't be listed until I refresh the page.
  - This works properly in I2A, though.
  - Thought: is there some code in I2A that refreshes the Classrooms Manager only after Assignments are auto-created? If so, that may explain why Darien has that quirk.
- [major] Delete Classroom button doesn't work on Darien. (EDIT: I2A too)

srallen · 2017-10-20T15:16:10Z

This needs a rebase now and delete classroom button sounds like a regression, so I'll work on patching that up. I'll take a look at the request for refreshing the classrooms list too for Darien.

shaunanoordin · 2017-10-20T15:26:09Z

src/containers/classrooms/ClassroomsTableContainer.jsx

-  selectClassroom(classroom) {
-    Actions.classrooms.selectClassroom(classroom);
+  determineClassroomsTable() {
+    const ClassroomsTable = this.props.selectedProgram.custom ? DarienClassroomsTable : AstroClassroomsTableContainer;


OK, now I see what you meant when you said "...after puzzling on this for a while I ended up settling with a ternary"

I think this is the way to go, at least for now. Re: "I'd really like to try to clean this up at some point", I'm all for it, but I'm OK to launch something that works first and refactor later. That said...

Thought: I imagine the ideal solution would be to properly plan out a kind of "generic Education Program" system similar to Panoptes' "generic Build-Your-Own Project" system, then plunk the switching code into a central "ProgramsController"/"ProgramsConfigToComponentTranslator" thingy...

switch (this.props.selectedProgram.settings.programType) { case "wildlife_camera_trap_classroom": ClassroomsTable = DarienClassroomTable; break; case "standard_auto_created_assignments_classroom": ClassroomsTable = AstroClassroomsTableContainer; break; ... } return ClassroomsTable;

...but that might be a pretty darn deep rabbit hole to go down.

It's a good idea, but not worth the time right now even though I dislike what I ended up doing! If another curricula/educational program is added after I2A and the Wildcams, then that's when this should be refactored.

shaunanoordin · 2017-10-20T15:29:28Z

src/containers/classrooms/ClassroomsTableContainer.jsx

+        assignmentsStatus={this.props.assignmentsStatus}
+        classrooms={this.props.classrooms}
+        match={this.props.match}
+        maybeDeleteClassroom={this.props.maybeDeleteClassroom}


Unsure: Is this missing deleteClassroom={this.props.deleteClassroom} ?

maybeDeleteClassroom={this.maybeDeleteClassroom} This should open the dialog and this.deleteClassroom is the click handler for confirmation. I missed removing the this.props part here.

shaunanoordin · 2017-10-20T15:32:56Z

src/containers/classrooms/ClassroomsManagerContainer.jsx

  render() {
    return (
      <ClassroomsManager
        classrooms={this.props.classrooms}
        classroomInstructions={this.props.classroomInstructions}
        classroomsStatus={this.props.classroomsStatus}
-        classroomToDelete={this.state.classroomToDelete}
-        closeConfirmationDialog={this.closeConfirmationDialog}
-        deleteClassroom={this.deleteClassroom}


Nyerp this one's bothering me, deleteClassroom is removed here, but I'm assuming it should have been added back/moved to somewhere else. I still can't tell where it's supposed to go, though.

That's right. I shuffled the classroom delete methods into ClassroomsTableContainer

It was my attempt at making sense of component organization and what should be responsible for what. Clearly, it's all still hard to follow 🤷‍♀️

srallen · 2017-10-20T15:44:10Z

I just rebased, so this should be a bit easier to see the changes specific to this PR.

shaunanoordin

PR Review

This PR adds classrooms to the WildCam Darien Program

The Darien Classrooms (under the Darien > Educator)
The Darien Classrooms are significantly different from the baseline I2A Classrooms, notably in that Assignments need to be manually created for each Classroom. (Note: this PR does not include Create Assignment forms yet.)

While the Darien Classroom Manager is in place, the two issues from the previous quick review are still extant.

Issues

When creating a new Classroom in Darien, that Classroom won't appear on the list until the page is refreshed.
User cannot delete Classrooms in either Darien or I2A; the popup doesn't appear after user clicks the X icon.

Status

Changes requested: 2 issues to fix.

However, if either of these fixes are meant to be tackled in a separate PR, please let me know.

srallen · 2017-10-23T18:45:11Z

@shaunanoordin those two issues should now be fixed. I found a bug with classroom creation, too, but I'll save that for another PR.

shaunanoordin

PR Review (Update)

The last update addresses both the issues from the last review - users can now delete Classrooms in both Darien and I2A, and new Classrooms in Darien are reflected properly on the list after creation. Nice work, @srallen ! 👍

Status

LGTM, this PR is good to go! Approving & merging.

srallen requested a review from shaunanoordin October 19, 2017 19:40

shaunanoordin reviewed Oct 20, 2017

View reviewed changes

Sarah Allen added 6 commits October 20, 2017 10:42

Add program filter

68c2945

Use non-spec program filter

312c3c5

Refactoring to enable Darien's classrooms table

d442e83

Add conditional for determine which classrooms table to use

296948f

Add forgotten files

baa3dca

Remove cruft

131a743

srallen force-pushed the darien-classrooms-table branch from 2592a33 to 131a743 Compare October 20, 2017 15:43

shaunanoordin requested changes Oct 23, 2017

View reviewed changes

Fix classroom delete regressions

e94145d

shaunanoordin approved these changes Oct 24, 2017

View reviewed changes

shaunanoordin merged commit 8777254 into master Oct 24, 2017

shaunanoordin deleted the darien-classrooms-table branch October 24, 2017 13:11

srallen mentioned this pull request Oct 24, 2017

Fix classroom form state #75

Merged

This was referenced Nov 10, 2017

Infinite routing loop on Darien classroom edit page #81

Closed

Darien edit classroom view -- shared component reorganization? #85

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Darien classrooms table #74

Darien classrooms table #74

srallen commented Oct 19, 2017

srallen commented Oct 19, 2017

shaunanoordin commented Oct 20, 2017

srallen commented Oct 20, 2017

shaunanoordin Oct 20, 2017

srallen Oct 20, 2017

shaunanoordin Oct 20, 2017

shaunanoordin Oct 20, 2017

srallen Oct 20, 2017

shaunanoordin Oct 20, 2017

srallen Oct 20, 2017

srallen Oct 20, 2017

srallen commented Oct 20, 2017

shaunanoordin left a comment

srallen commented Oct 23, 2017

shaunanoordin left a comment

Darien classrooms table #74

Darien classrooms table #74

Conversation

srallen commented Oct 19, 2017

srallen commented Oct 19, 2017

shaunanoordin commented Oct 20, 2017

srallen commented Oct 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srallen commented Oct 20, 2017

shaunanoordin left a comment

Choose a reason for hiding this comment

PR Review

Issues

Status

srallen commented Oct 23, 2017

shaunanoordin left a comment

Choose a reason for hiding this comment

PR Review (Update)

Status