Editable project instructions by ajgreenb · Pull Request #1387 · popcodeorg/popcode

ajgreenb · 2018-03-09T18:06:05Z

A basic first pass at making the Popcode instructions panel editable from within the app.

To Do

Add a Save button that updates the project's instructions state.
Add tests! 🙂
Add "Markdown supported" blurb at the bottom of InstructionsEditor.
See if it's possible to make the cursor: default for the instructions bar when in editing mode.
Polish up the styling.

Closes #1311

outoftime · 2018-03-09T18:26:37Z

Nice! I think it makes sense for @jwang1919 to do the first review pass on this?

jwang1919 · 2018-03-10T16:08:20Z

Will see if I have time later to review later today. I will be out next week for work.

jwang1919

Some minor change requests and comments on functionality regarding the Hamburger menu and when isEditingInstructions is active.

jwang1919 · 2018-03-10T17:38:50Z

    "export-gist": "Export Gist",
    "export-repo": "Export Repo",
    "share-to-classroom": "Share To Classroom",
+    "add-or-edit-instructions": "Add/Edit Instructions",


I prefer having "Add Instructions" and "Edit Instructions" as two different strings. And have these strings show up whether or not instructions are present in the Popcode project.

jwang1919 · 2018-03-10T17:40:21Z

-      "output": "Output"
+      "output": "Output",
+      "instructions": {
+        "cancel": "Cancel"


Not quite sure why this string is separate from "add-or-edit-instructions".
Couldn't we do...

"instructions": { "add": "Add Instructions", "edit": "Edit Instructions", "cancel": "Cancel" }

Purposely omitted "Save" because you haven't accomplished that yet, but same deal as with the other strings.

I separated them because add-or-edit-instructions (soon to be split into two separate strings) is in the top-bar, while cancel and save are in the Instructions component. But I don't feel strongly about that separation, so I'm happy to group them all together if you think that would be better.

jwang1919 · 2018-03-10T17:52:00Z

+  }
+}
+
+.instructions-editor-menu {


Suggest to follow BEM naming rules like the rest of the selectors.
.instructions-editor-menu to .instructions-editor__menu

jwang1919 · 2018-03-10T17:52:52Z

+export default function InstructionsEditor({instructions, onCancelEditing}) {
+  return (
+    <div className="instructions-editor">
+      <div className="instructions-editor-menu">


Suggest to follow BEM naming rules like the rest of the selectors.
"instructions-editor-menu" to "instructions-editor__menu"

jwang1919 · 2018-03-10T18:03:09Z

+        items.push(
+          <MenuItem
+            key="addOrEditInstructions"
+            onClick={isEditingInstructions ? noop : onStartEditingInstructions}


When isEditingInstructions is true, I think we should change the Menu Item in someway so it's clear that this menu item is active.

My ideas:

Disable the Edit Menu Item

Change the text to "Stop Editing Instructions" (this will cancel and close the instructions)

Change the text to "Save Instructions" (this will save and close the instructions).

ajgreenb · 2018-03-11T08:06:07Z

@jwang1919 I think I've addressed all your feedback (on one point I added a comment, but GitHub has collapsed it.)

I decided to opt for idea 1 when isEditingInstructions is true (disabling the menu item.) My reasoning is that

It's not obvious whether saving or canceling should be the behavior of the menu item when isEditingInstructions is true.
Saving and canceling are already afforded by the buttons in the InstructionsEditor and I've heard it's good to only have one way to do something when that's possible.

Let me know what you think!

ajgreenb · 2018-03-11T08:10:03Z

Also, I added the ability to save the new instructions using a contenteditable pre. It works fine, but React is a little grudging:

Warning: A component is `contentEditable` and contains `children` managed by React. It is now your responsibility to guarantee that none of those nodes are unexpectedly modified or duplicated. This is probably not intentional.

The contentious "nodes" in this case aren't actually React elements. It's just a String. How do y'all feel about this?

ajgreenb · 2018-03-11T08:58:04Z

Added a couple tests—let me know if there are any more you'd like to see!

outoftime · 2018-03-11T15:11:05Z

@ajgreenb what’s the reasoning behind the contenteditable rather than just using a <textarea>?

ajgreenb · 2018-03-11T15:23:20Z

I think it gets the job done more simply than using a textarea. With a textarea, I'd have to add more state management for state that is "ephemeral" in a sense. From my understanding, the goal is to to allow users to save and cancel, so I'd need to create separate state elements for both instructions and currentInstructionsBuffer (a better name, hopefully.) When the user saves, currentInstructionsBuffer gets copied to instructions and when the user cancels, it gets discarded.

Using contenteditable, that behavior can be contained within the InstructionsEditor component, which feels less leaky to me.

outoftime · 2018-03-11T16:43:18Z

@ajgreenb so it looks like what you’ve got here is basically an uncontrolled component implementation using a contenteditable element—transient edit state lives purely in the DOM, and then is pulled out of a ref when it’s time to save changes. What you’re describing doing with a <textarea> is a controlled component implementation, which is indeed the preferred approach to forms in React.

However, there’s no reason you couldn’t have an uncontrolled <textarea>—in fact, I think that would be even simpler, since you could avoid messing with state at all, and just set the textarea’s defaultValue to the instructions prop.

WDYT? Definitely possible there is some subtlety I am missing here…

ajgreenb · 2018-03-11T18:12:18Z

Ah, yeah of course. It’s been a while since I worked with React so I had forgotten about uncontrolled components. I’ll make that change; thanks for the suggestion!

pwjablonski · 2018-03-12T21:45:21Z

@ajgreenb Checked this out - this is so awesome! Not sure if you addressed this already or have it on the backlog- I found myself wanting to have the "edit instructions" button appear in the instructions pane rather than having to go back to the hamburger menu. That way I could make changes to the markdown => save => edit to fix changes more quickly. I think "add instructions" should still live in the hamburger menu though. Thoughts?

ajgreenb · 2018-03-12T22:12:58Z

That sounds like a great idea! I’ll try implementing it as soon as I have some time. Thanks for the suggestion!

pwjablonski · 2018-03-12T22:21:11Z

Cool! The upside to the current implementation is that the edit instructions button is less accessible to students under the hamburger menu. But as an instructor much prefer it in the instructions pane.

outoftime · 2018-03-13T23:07:07Z

I think a fairly subtle (maybe only appearing on hover?) edit icon in the instructions would be the move here—not super eye-catching if you’re not looking for it, but easily accessible if you’re looking doing a quick edit-save-edit-save workflow. Anyway, seems like we could follow up with that.

@ajgreenb @jwang1919 please let me know when you’re ready for me to do final review on this—would love to get it released! Very excited to never export a gist just to add an INSTRUCTIONS.md again : D

ajgreenb · 2018-03-16T00:10:54Z

Okay, in the interest of getting this out, let's leave the edit icon in the instructions for a small follow-up PR. This is pretty close to ready for final review—hopefully I'll have some time this weekend to put the final touches on it.

ajgreenb · 2018-03-16T22:24:27Z

@outoftime I think this is ready for feedback! The styles could use some love, but they don't need to block a review.

outoftime

@ajgreenb this looks great!! Left a couple of low-level bits of feedback and some thoughts on styles/CSS, but overall this feels very close to the launch tubes!

outoftime · 2018-03-21T00:51:38Z

+  }
+
+  _ref(editorElement) {
+    this.editor = editorElement;


We should underscore-prefix this._editor as well as it is “private” state

outoftime · 2018-03-21T00:52:12Z

        </MenuItem>,
      );

+      if (isExperimental) {


I don’t think there’s any need to release this in experimental mode? Seems pretty ready for prime time to me!

outoftime · 2018-03-21T00:54:37Z

+          <MenuItem
+            isDisabled={isEditingInstructions}
+            key="addOrEditInstructions"
+            onClick={isEditingInstructions ? noop : onStartEditingInstructions}


Shouldn’t the isDisabled prop be sufficient to make onClick a noop?

outoftime · 2018-03-21T00:57:12Z

-      get(this.props, ['currentProject', 'projectKey']),
-      'instructions',
-    ));
+    if (!this.props.isEditingInstructions) {


Small thing, but if clicking the instructions bar doesn’t do anything when editing is enabled (which seems sensible), I think we should take away the cursor: pointer in that case too : )

Oh lol this is one of your pending checklist items!

outoftime · 2018-03-21T00:59:34Z

+  'START_EDITING_INSTRUCTIONS',
+);
+
+export const stopEditingInstructions = createAction(


I don’t feel strongly about this, but I think cancelEditingInstructions might be a clearer name here—stopEditingInstructions intuitively feels like it would be dispatched any time you are done editing instructions, whether or not you’re going to save your changes.

outoftime · 2018-03-21T01:03:57Z

+
+  /* postcss-bem-linter: ignore */
+  & textarea {
+    height: 100%;


This doesn’t seem to be having the desired effect in Chrome 65—the editor is just a couple lines high. I find absolute positioning with all four sides pinned works wonders in this kind of situation ; )

outoftime · 2018-03-21T01:04:15Z

+}
+
+.instructions-editor__input {
+  padding: 1rem 1rem 0;


I think we could do with less padding, and maybe none at all?

outoftime · 2018-03-21T01:05:03Z

+    width: 100%;
+    padding: 0.5rem;
+    box-sizing: border-box;
+    font-family: monospace;


Let’s use Roboto to match the code editors?

outoftime · 2018-03-21T01:08:23Z

+.instructions-editor__footer {
+  margin: 1rem;
+  padding-top: 1rem;
+  border-top: 1px solid var(--color-light-gray);


I think this text could stand to be a bit smaller.

outoftime · 2018-03-21T01:10:50Z

+          />
+        </div>
+        <div className="instructions-editor__footer">
+          Styling with Markdown is supported


Let’s make this a link to a suitable Markdown guide?

This commit also extracts the editable pre element into a separate InstructionsEditor React component.

ajgreenb · 2018-03-21T21:56:04Z

@outoftime I think I addressed all your feedback! Let me know what you think. I'm not in love with styling of the Save and Cancel buttons (lack thereof)—should I spend some time trying to make them look more like the buttons in the top bar?

outoftime

@ajgreenb awesome!! gonna release this now. a couple of things i think would be good for a follow up:

It’d be nice if the edit textarea were focused when it’s first rendered (especially when there are no preexisting instructions, it’s not immediately visually obvious what’s going on)
Along the same lines, perhaps a placeholder in the textarea?
I meant to suggest we use Hack (the editor font) rather than Roboto (the UI font) in the textarea. My bad!!!
I think we should consider renaming the isEnabled prop that MenuItem takes—it’s confusing to have both isEnabled and isDisabled, talking about different things. Maybe isActive instead of isEnabled?

outoftime requested a review from jwang1919 March 9, 2018 18:26

outoftime added the in progress label Mar 9, 2018

outoftime changed the title ~~[WIP] Editable project instructions~~ Editable project instructions Mar 9, 2018

jwang1919 reviewed Mar 10, 2018

View reviewed changes

ajgreenb force-pushed the editable-project-instructions branch from 1c2d44f to 3f986c0 Compare March 16, 2018 22:23

ajgreenb force-pushed the editable-project-instructions branch from 3f986c0 to 459cdf1 Compare March 17, 2018 19:39

outoftime requested changes Mar 21, 2018

View reviewed changes

greenberga added 8 commits March 21, 2018 17:53

Add hamburger menu button for editing Instructions

599778e

Add state management logic for instructions

afaedb7

Wire hamburger menu to instructions editing state

027f35b

Create basic instructions editor

174a993

Add a (stubbed) Cancel button

3dfba1b

This commit also extracts the editable pre element into a separate InstructionsEditor React component.

Wire cancel button to instructions state mgmt

2dd3edc

Use i18n functionality for button text

54487ad

Add state mgmt logic for updating instructions

f56e959

greenberga added 15 commits March 21, 2018 17:53

Wire editor save button to state mgmt logic

b07fc88

Fix CSS BEM naming violations

bb8eb7f

Toggle instructions hamburger menu item on state

1c958f7

Add tests for new instructions reducer logic

9d52744

Use uncontrolled textarea for InstructionsEditor

e451ea6

Add a Markdown blurb footer thingy

fd6dd05

Add _ prefix to this.editor to signify privateness

bb99df4

Remove Instructions editing from experimental

13a90f0

Noop MenuItem clicks when isDisabled == true

5ab05fb

Show default cursor on Instruct bar when editing

9136733

Rename stopEditingInstructions to cancelEditing...

5ef4cfb

Remove unused CSS variable

2576d2e

Remove shading of disabled menu item on hover

2a9a2ea

Fix styling of InstructionsEditor

b3c7da8

Link to Markdown syntax instructions

667fd47

ajgreenb force-pushed the editable-project-instructions branch from 936ef74 to 667fd47 Compare March 21, 2018 21:53

outoftime approved these changes Mar 23, 2018

View reviewed changes

outoftime merged commit 310dba8 into popcodeorg:master Mar 23, 2018

Conversation

ajgreenb commented Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To Do

Uh oh!

outoftime commented Mar 9, 2018

Uh oh!

jwang1919 commented Mar 10, 2018

Uh oh!

jwang1919 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajgreenb commented Mar 11, 2018

Uh oh!

ajgreenb commented Mar 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajgreenb commented Mar 11, 2018

Uh oh!

outoftime commented Mar 11, 2018

Uh oh!

ajgreenb commented Mar 11, 2018

Uh oh!

outoftime commented Mar 11, 2018

Uh oh!

ajgreenb commented Mar 11, 2018 via email

Uh oh!

pwjablonski commented Mar 12, 2018

Uh oh!

ajgreenb commented Mar 12, 2018 via email

Uh oh!

pwjablonski commented Mar 12, 2018

Uh oh!

outoftime commented Mar 13, 2018

Uh oh!

ajgreenb commented Mar 16, 2018

Uh oh!

ajgreenb commented Mar 16, 2018

Uh oh!

outoftime left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajgreenb commented Mar 9, 2018 •

edited

Loading

ajgreenb commented Mar 11, 2018 •

edited

Loading