Inline edit controls - start editing and cancel editing #20029

caleybrock · 2018-01-16T00:05:05Z

Edit button has an action that puts rows in editing state.
Password section has an editing state.
Cancel button stops editing.
Add redux tests for new actions and fix storybook

Bjvanminnen · 2018-01-16T20:42:38Z

Is it weird that here we click the quick actions menu, select edit, and get an inline experience. But on the homepage, for the sections table, if we click on the quick actions menu, select "Edit section" we get a modal edit experience?

Bjvanminnen · 2018-01-16T20:43:09Z

apps/src/templates/manageStudents/ManageStudentsActionsCell.jsx

@@ -4,6 +4,8 @@ import PopUpMenu, {MenuBreak} from "@cdo/apps/lib/ui/PopUpMenu";
 import color from "../../util/color";
 import FontAwesome from '../FontAwesome';
 import Button from '../Button';
+import {startEditingStudent, cancelEditingStudent} from './manageStudentsRedux';
+import { connect } from 'react-redux';


nit: would be good to be consistent on whether or not we have styles before/after braces in a single file

Bjvanminnen · 2018-01-16T20:45:28Z

apps/src/templates/manageStudents/ManageStudentsActionsCell.jsx

@@ -15,6 +17,17 @@ class ManageStudentActionsCell extends Component {
  static propTypes = {
    id: PropTypes.number.isRequired,
    isEditing: PropTypes.bool,
+    //Provided by redux


supernit: throughout most of our code, we have a space after beginning a comment (though not actually true everywhere and it doesn't actually matter that much)

Bjvanminnen · 2018-01-16T20:46:49Z

apps/src/templates/manageStudents/ManageStudentsActionsCell.jsx

+  cancelEditingStudent(id) {
+    dispatch(cancelEditingStudent(id));
+  },
+}))(ManageStudentActionsCell);


You can actually just use a shortcut version of matchPropsToDispatch:

export default connect(state => {}, {startEditingStudent, cancelEditingStudent})(ManageStudentActionsCell)

This is really good to know, although way less readable to me, so I'm going to leave it as is.

I find it interesting to hear that you find the short form less readable (though I'm absolutely fine with you leaving this as is given that you think so).

Bjvanminnen · 2018-01-16T20:47:33Z

apps/src/templates/manageStudents/ManageStudentsTable.story.jsx

@@ -1,6 +1,15 @@
 import React from 'react';
 import {UnconnectedManageStudentsTable} from './ManageStudentsTable';
 import {SectionLoginType} from '@cdo/apps/util/sharedConstants';
+import {combineReducers, createStore} from 'redux';
+import manageStudents from './manageStudentsRedux';
+import { Provider } from 'react-redux';


nit: similar comment about brace/space consistency

Bjvanminnen · 2018-01-16T20:48:20Z

apps/src/templates/manageStudents/manageStudentsRedux.js

@@ -31,13 +35,33 @@ export default function manageStudents(state=initialState, action) {
      studentData: action.studentData,
    };
  }
+  if (action.type === START_EDITING_STUDENT) {
+    let updatedStudentData = state.studentData;
+    updatedStudentData[action.studentId].isEditing = true;


This is mutating existing state (updatedStudentData is a reference to state.studentData, not a copy of it)

Bjvanminnen · 2018-01-16T20:49:57Z

apps/src/templates/manageStudents/manageStudentsRedux.js

+    return {
+      ...state,
+      studentData: updatedStudentData,
+    };


I think a way to do this without mutating (that isn't super understandable) is

return { ...state, studentData: { ...state.studentData, [action.studentId]: { ...state.studentData[action.studentId], isEditing: true } } };

Same issue for cancel

Bjvanminnen · 2018-01-16T20:51:24Z

apps/src/templates/manageStudents/manageStudentsRedux.js

+  let studentDataArray = [];
+  for (var id in studentData) {
+    studentDataArray.push(studentData[id]);
+  }


I think this can just be Object.values(studentDataArray)?

Bjvanminnen · 2018-01-16T20:51:43Z

apps/src/templates/manageStudents/manageStudentsRedux.js

+
+// Converts key/value id/student pairs to an array of student objects for the
+// component to display
+// TODO: memoize this - sections could be a few thousand students


Should attach name to TODO

Bjvanminnen

Mostly nits, but will want to fix mutating state in the reducer.

caleybrock · 2018-01-16T20:57:56Z

I asked the same question about the edit experience. Many of the fields in the sections table require explanatory text, where as the student table fields are self explanatory and require quick editing, often of many rows at a time. Making this table have a dialog would be a worse experience for teachers, and making the sections table inline would probably cause confusion.

Bjvanminnen · 2018-01-16T21:08:18Z

I would agree that it makes sense for these tables to have different edit experiences. I wonder if they should also have different ways to begin the edit experience? Prob not a big deal either way.

caleybrock · 2018-01-16T22:44:02Z

PTAL

Bjvanminnen · 2018-01-16T22:54:05Z

apps/src/templates/manageStudents/manageStudentsRedux.js

+          isEditing: true
+        }
+      }
+    };


Note: An alternative to this would be to use Immutable.js, which I think would allow you to do something like

return state.set(['studentData', action.studentId, isEditing'], true);

Doing some googling, I think you could also accomplish this with some lodashing, doing something like:

return _.set(_.clone(state), `studentData.${action.studentId}.isEditing`, true);

I haven't validated that this works, and am mostly just sharing as an FYI. The pattern currently used here is one we use elsewhere

Turns out this was bad advice. Clone only makes a shallow copy, so this still ends up doing a mutation.

Consider the following code:

var foo = { parent: { x: 1, y: 2 } }; console.log(foo.parent.x); // prints 1 var foo2 = _.set(_.clone(foo), 'parent.x', 3); console.log(foo.parent.x); // prints 3, foo and _.clone(foo) are both using the same parent object

The following does appear to work, but I'm not sure I grok it well enough to recommend it's usage:

var foo2 = _.setWith(_.clone(foo), 'parent.x', 3, _.clone)

alright, but it sounds like they way it is currently works.

Yep. Current approach is 👍

caleybrock added 4 commits January 15, 2018 14:25

Convert redux store to key/value pairs with converters for the component

ec9344a

Start editing and cancel editing

39b8f91

Add test for start and cancel editing

3e01b51

Fix storybook

6533b2f

caleybrock requested a review from Bjvanminnen January 16, 2018 18:32

Bjvanminnen reviewed Jan 16, 2018

View reviewed changes

Code review feeback for ManageStudents

4ba9622

Merge remote-tracking branch 'origin/staging' into inline-edit-controls

3ecb6be

Bjvanminnen reviewed Jan 16, 2018

View reviewed changes

Bjvanminnen approved these changes Jan 16, 2018

View reviewed changes

caleybrock merged commit a5351ef into staging Jan 17, 2018

caleybrock deleted the inline-edit-controls branch January 17, 2018 00:47

caleybrock added the star-labs label Feb 28, 2018

caleybrock added teacher-dashboard and removed star-labs labels May 3, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inline edit controls - start editing and cancel editing #20029

Inline edit controls - start editing and cancel editing #20029

caleybrock commented Jan 16, 2018 •

edited

Bjvanminnen commented Jan 16, 2018

Bjvanminnen Jan 16, 2018

Bjvanminnen Jan 16, 2018

Bjvanminnen Jan 16, 2018

caleybrock Jan 16, 2018

Bjvanminnen Jan 16, 2018

Bjvanminnen Jan 16, 2018

Bjvanminnen Jan 16, 2018

Bjvanminnen Jan 16, 2018

Bjvanminnen Jan 16, 2018

Bjvanminnen Jan 16, 2018

Bjvanminnen Jan 16, 2018

Bjvanminnen left a comment

caleybrock commented Jan 16, 2018

Bjvanminnen commented Jan 16, 2018

caleybrock commented Jan 16, 2018

Bjvanminnen Jan 16, 2018

caleybrock Jan 16, 2018

Bjvanminnen Jan 18, 2018

Bjvanminnen Jan 18, 2018

caleybrock Jan 18, 2018

Bjvanminnen Jan 18, 2018

Inline edit controls - start editing and cancel editing #20029

Inline edit controls - start editing and cancel editing #20029

Conversation

caleybrock commented Jan 16, 2018 • edited

Bjvanminnen commented Jan 16, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bjvanminnen left a comment

Choose a reason for hiding this comment

caleybrock commented Jan 16, 2018

Bjvanminnen commented Jan 16, 2018

caleybrock commented Jan 16, 2018

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

caleybrock commented Jan 16, 2018 •

edited