Add a WYSIWYG-esque editor for instructions #1426

meg2208 · 2018-04-22T21:33:57Z

This closes #1394 and #1397. Please see gif below for a demo. :)

meg2208 · 2018-04-23T00:56:08Z

@outoftime The only thing I still would like to add here is the async component loading we were discussing. I was going to use this article but not sure if react-lazy-load or react-async-componet is better. Did you say there is somewhere in the project you are already doing this?

outoftime · 2018-04-23T12:03:08Z

@meg2208 Sick! Just took a quick pass through the code and looks great. Hopefully I’ll be able to do a close review this evening.

As for the two async libraries—we don’t currently do any async rendering; we do use async loading elsewhere in the code, though it’s more straightforward when you’re not dealing with component rendering.

Glancing at the two libraries’ READMEs, I’d be inclined to go with react-async-component—react-lazy-load seems to be mostly concerned with lazy load on scroll, which while useful in a lot of contexts, isn’t what we need here. react-async-component looks just right—simply defer the loading of a component’s code until the first time we want to render the component. Definitely open to other arguments though!

outoftime

@meg2208 nice work on this! left a handful of low-level feedback but the high-level structure here looks just right. can’t wait to release this!

outoftime · 2018-04-23T12:05:40Z

src/actions/ui.js

@@ -65,6 +65,12 @@ export const startEditingInstructions = createAction(
  (_projectKey, timestamp = Date.now()) => ({timestamp}),
 );

+export const continueEditingInstructions = createAction(


I wasn’t sure what continueEditingInstructions was based on the name (had to look at where it was used)—maybe we could do something more explicit/verbose like updateInProgressInstructions?

This is of course the camel case version of the action name, and for the action I was following the pattern of the actions {verb}_EDITING_INSTRUCTIONS. Other verbs are start and cancel. You would rather break that pattern? It just seems a bit less intuitive but I'm fine with it. So it would be UPDATE_IN_PROGRESS_INSTRUCTIONS? I figure continueEditingInstructions fits in nicely with startEditingInstructions and cancelEditingInstructions. Another idea for something more verbose is continueEditingUnsavedInstructions.

Definitely understand the appeal of the symmetry, but ultimately I think clarity/expressiveness is the overriding concern here. Given that you went with draftInstructions for the property name (which I like) I would propose updateDraftInstructions here. WDYT?

outoftime · 2018-04-23T12:06:28Z

src/components/InstructionsEditor.jsx

-      this._editor.focus();
-    }
+    bindAll(this, '_handleCancelEditing', '_handleContinueEditing',
+      '_handleSaveChanges');


Project style for a multi-line function call is one argument per line:

bindAll( this, '_handleCancelEditing', '_handleContinueEditing', '_handleSaveChanges', );

outoftime · 2018-04-23T12:43:09Z

src/css/application.css

@@ -865,3 +865,11 @@ body {
 .u__icon_disabled {
  cursor: default;
 }
+


The project’s CSS bundling system certainly lacks elegance, but we do want to avoid pasting dependency styles into the first-party stylesheet.

Ideal would be to avoid loading these styles until the instructions editor needs them, maybe something with webpack CSS modules? But probably beyond the scope of this PR.

Most straightforward would be to directly reference the CSS file in the simplemde package, as we do for normalize.css.

outoftime · 2018-04-23T12:49:39Z

src/actions/ui.js

+export const continueEditingInstructions = createAction(
+  'CONTINUE_EDITING_INSTRUCTIONS',
+  (projectKey, content) => ({projectKey, content}),
+  (_projectKey, timestamp = Date.now()) => ({timestamp}),


Do we use meta.timestamp from this action? We can save ourselves the trouble of generating it if not.

outoftime · 2018-04-23T12:51:35Z

src/reducers/ui.js

@@ -11,6 +11,7 @@ export const DEFAULT_WORKSPACE = new Immutable.Map({
  columnFlex: DEFAULT_COLUMN_FLEX,
  rowFlex: DEFAULT_ROW_FLEX,
  isDraggingColumnDivider: false,
+  displayedInstructions: '',


This property name might be a bit confusing—I could see it describing the instructions that you see on the screen when just viewing instructions, outside the context of editing.

Maybe something like inProgressInstructions, draftInstructions, etc.?

outoftime · 2018-04-23T13:19:49Z

src/selectors/getCurrentProjectInstructions.js

+    }
+
+    if (isEditing && instructionUnsaved) {
+      return instructionUnsaved;


So what’s the thinking behind changing the semantics of this selector? I’m a little wary of this as the more complex behavior may not be obvious/expected from the call site. If there are situations in which we do want a selector for “bleeding edge instructions” I would propose creating a new selector whose name describes its behavior more explicitly. But I’m down to be convinced otherwise!

I did add getCurrentProjectinstructionsUnsaved, and note that this selector invokes that one, but I did like that this was all pulled when the HOC is composed like this:

function mapStateToProps(state) { return { instructions: getCurrentProjectInstructions(state), isEditing: isEditingInstructions(state), isOpen: !getHiddenUIComponents(state).includes('instructions'), projectKey: getCurrentProjectKey(state), }; }

So this selector sort of becomes the router on which instructions to pull, without having to change the existing logic or add another wrapper on top of those two. I think if this needs to get any more complex in the future it would make sense to break it out, but for now I added some comments explaining. If you still want me to update it, happy to do it 👍

I feel you on the appeal of having a “router” selector for sure. My concern is just making this selector that router—the name implies the more straightforward previous behavior. An example of where this would be an issue is if getCurrentProjectInstructions were used somewhere else in the application to decide whether to display a “Delete Instructions” button (based on the selector returning a truthy value). This would yield weird behavior as currently written.

So, I would propose creating a new selector that composes getCurrentProjectInstructions and getCurrentProjectInstructionsUnsaved (although again I would go for something like getCurrentProjectDraftInstructions), and using that here when we explicitly want the “routing behavior”.

That said, I am also not totally convinced we want to be routing the unsaved instructions through the instructions prop in the <Instructions> component. Might it be simpler to reason about if the component were explicitly given the instructions for display and the draft instructions for the editor as separate props? In fact, it seems like a draftInstructions prop could also do the job of isEditing, i.e. the mere presence of draftInstructions would be the signal that we are currently editing.

WDYT?

All good points. I would be fine with putting it in the render of the <Instructions> component and passing both values in. It's really just a one-liner.

outoftime · 2018-04-23T13:20:05Z

src/selectors/getCurrentProjectInstructions.js

-    projectKey ? projects.getIn([projectKey, 'instructions']) : '',
+  [
+    getCurrentProjectKey, getProjects,
+    isEditingInstructions, getCurrentProjectInstructionsUnsaved,


If you agree with my below comment then probably not relevant, but project style would be one array element per line.

outoftime · 2018-04-23T13:20:32Z

src/selectors/getCurrentProjectInstructionsUnsaved.js

@@ -0,0 +1,3 @@
+export default function getCurrentProjectinstructionsUnsaved(state) {


Small thing, but I think getCurrentProjectUnsavedInstructions would read a little more naturally.

...unless you are Yoda.

I did it to be more inline alphabetically with the other selectors (see below) but can update it, it just won't be as pretty in the IDE. :)

Yep, I feel you, but I think clarity is again the first-order concern : )

outoftime · 2018-04-24T12:43:46Z

src/css/simple-mde.css

+ * Copyright Next Step Webs, Inc.
+ * @link https://github.com/NextStepWebs/simplemde-markdown-editor
+ * @license MIT
+ */


Ideally we’d like to avoid ever having to vendor dependencies into source control—package managers are of course much easier to work with. In this case I think we should just be able to add 'node_modules/simplemde/dist/simplemde.min.css' to this list. We’ll also want to add simplemde itself as a dependency in package.json so we can guarantee that it will be hoisted to the top level, but that seems fine.

outoftime · 2018-04-24T12:51:28Z

src/selectors/getCurrentProjectInstructions.js

+    }
+
+    if (isEditing && instructionUnsaved) {
+      return instructionUnsaved;


I feel you on the appeal of having a “router” selector for sure. My concern is just making this selector that router—the name implies the more straightforward previous behavior. An example of where this would be an issue is if getCurrentProjectInstructions were used somewhere else in the application to decide whether to display a “Delete Instructions” button (based on the selector returning a truthy value). This would yield weird behavior as currently written.

So, I would propose creating a new selector that composes getCurrentProjectInstructions and getCurrentProjectInstructionsUnsaved (although again I would go for something like getCurrentProjectDraftInstructions), and using that here when we explicitly want the “routing behavior”.

That said, I am also not totally convinced we want to be routing the unsaved instructions through the instructions prop in the <Instructions> component. Might it be simpler to reason about if the component were explicitly given the instructions for display and the draft instructions for the editor as separate props? In fact, it seems like a draftInstructions prop could also do the job of isEditing, i.e. the mere presence of draftInstructions would be the signal that we are currently editing.

WDYT?

outoftime

GitHub is being a little annoying about comments and diffs but please see:

Feels like we’re getting close! Excited to see what the async rendering looks like.

outoftime · 2018-08-12T15:42:35Z

@meg2208 heyo—I’d love to get this feature released and it feels pretty close! the async loading question came up in another context as well recently and would be great to have your solution in here as a template. any chance you’ve got some time to bring this one home??

meg2208 added 5 commits April 22, 2018 15:22

add react-simplemde-editor dependency

3073cc9

WYSIWYG-esque editor for instructions; controlled component pattern

c1ad7d8

minor styling for instructions editor

2864084

proper behavior on instructions cancel, load new project

1544c8e

add newline; remove unnecessary id

c81a8cf

outoftime requested changes Apr 23, 2018

View reviewed changes

meg2208 added 3 commits April 23, 2018 22:13

move css to simple-mde.css; updates from code review

4bfae0f

add logic explanation to getCurrentProjectInstructions

e2588eb

displayedInstructions --> draftInstructions

04a6912

meg2208 mentioned this pull request Apr 24, 2018

Upgrade to latest react version (16.3.2) #1427

Closed

fix lint error

903ad12

meg2208 mentioned this pull request Apr 24, 2018

upgrade to react 16.3 #1428

Merged

disable stylelint for venor css

d911f6c

outoftime reviewed Apr 24, 2018

View reviewed changes

outoftime requested changes Apr 24, 2018

View reviewed changes

meg2208 added 2 commits April 24, 2018 10:44

Merge branch 'master' into instructions-WYSIWYG

229e3e1

add react-coroutine, async load InstructionsEditor and fade in

19a5f33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a WYSIWYG-esque editor for instructions #1426

Add a WYSIWYG-esque editor for instructions #1426

meg2208 commented Apr 22, 2018

meg2208 commented Apr 23, 2018

outoftime commented Apr 23, 2018

outoftime left a comment

outoftime Apr 23, 2018

meg2208 Apr 24, 2018

outoftime Apr 24, 2018

outoftime Apr 23, 2018

outoftime Apr 23, 2018

outoftime Apr 23, 2018

outoftime Apr 23, 2018

outoftime Apr 23, 2018

meg2208 Apr 24, 2018

outoftime Apr 24, 2018

meg2208 Apr 25, 2018

outoftime Apr 23, 2018

outoftime Apr 23, 2018

meg2208 Apr 24, 2018

outoftime Apr 24, 2018

outoftime Apr 24, 2018

outoftime Apr 24, 2018

outoftime left a comment

outoftime commented Aug 12, 2018

		@@ -0,0 +1,3 @@
		export default function getCurrentProjectinstructionsUnsaved(state) {

Add a WYSIWYG-esque editor for instructions #1426

Are you sure you want to change the base?

Add a WYSIWYG-esque editor for instructions #1426

Conversation

meg2208 commented Apr 22, 2018

meg2208 commented Apr 23, 2018

outoftime commented Apr 23, 2018

outoftime left a 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

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

outoftime left a comment

Choose a reason for hiding this comment

outoftime commented Aug 12, 2018