Draggable console #1310

pwjablonski · 2017-12-24T04:31:36Z

This PR adds a draggable divider to the console. Definitely a rough first pass but got a working sample here. Looking forward to feedback : )

pwjablonski · 2017-12-24T04:32:53Z

src/components/Console.jsx

-  }
+  // if (!isEnabled) {
+  //   return null;
+  // }


Forgot to uncomment this!

pwjablonski · 2017-12-24T04:33:26Z

src/components/Console.jsx

@@ -3,6 +3,7 @@ import partial from 'lodash/partial';
 import React from 'react';
 import PropTypes from 'prop-types';
 import ImmutablePropTypes from 'react-immutable-proptypes';
+import prefixAll from 'inline-style-prefixer/static';


Not quite sure what this does but I saw it in the other implementations

@pwjablonski the application.css file is run though autoprefixer, which adds vendor prefixes for older browsers that don’t support the unprefixed version of more modern CSS features (e.g. -webkit-flex for flex). However, in the case we need to write styles directly into our JSX, autoprefixer doesn’t have a shot at the styles we’re writing. So we use inline-style-prefixer to do the same thing to inline styles.

pwjablonski · 2017-12-24T04:34:29Z

src/components/Console.jsx

@@ -43,7 +46,12 @@ export default function Console({
  const chevron = isOpen ? ' \uf078' : ' \uf077';

  return (
-    <div className="console">
+    <div
+      className={isOpen ? 'output__row console' :


Should use classnames

outoftime

Peter you are truly unstoppable! Left a couple of pieces of pretty high-level feedback here; let me know what you think. Assuming you want to move forward with either of both of those I’m going to hold off on a more fine-toothed review until then.

outoftime · 2017-12-24T17:21:14Z

src/actions/index.js

@@ -27,6 +27,10 @@ import {
  dragColumnDivider,
  startDragColumnDivider,
  stopDragColumnDivider,
+  dragOutputDivider,
+  startDragOutputDivider,
+  stopDragOutputDivider,


Hate to keep doing this to you but it looks like we’ve it another “rule of three” here—three different cases where we want to be doing basically the same thing (handling the dragging of a divider and updating some flex bases accordingly). So, I think we should have a single set of actions to handle divider dragging, and just store the flex bases in a map with keys like editors, columns, and output.

Yeah I was wondering about that. Can update this.

outoftime · 2017-12-24T17:26:22Z

src/components/Preview.jsx

+    <div
+      className="output__row preview"
+      ref={onRef}
+      style={isConsoleOpen ? prefixAll({flex: outputColumnFlex[0]}) : null}


In the other draggable flex implementations, the child components / flex items are passed their specific flex, rather than the entire data structure, which seems preferable. Here the <Preview/> is required to know that it’s the first flex item, which increases coupling.

Great I will update this

outoftime · 2017-12-24T17:40:18Z

src/components/Workspace.jsx

@@ -62,6 +72,9 @@ class Workspace extends React.Component {
      '_handleClickInstructionsBar',
      '_handleComponentUnhide',
      '_handleComponentHide',
+      '_handleOutputDividerDrag',
+      '_handleOutputDividerStart',
+      '_handleOutputDividerStop',


The <Workspace> component is basically a big ball of legacy code—it used to be the only connected component in the entire application, requiring all data to flow through it, making it an even bigger mess than it is today.

I started a big refactor to add containers (connected components), although I didn’t entirely finish. In particular, the <EditorsColumn> should have a container, but doesn’t. The end goal of the refactor would be for <Workspace> to be a purely presentational component (probably with its own container to handle a couple of things).

In general I would like to avoid adding new data flow paths to <Workspace>, as it brings us further from the goal of being purely presentational. I generally make an exception for things that need to support the <EditorsColumn>, because that’s just an unfinished bit of the container restructuring. But in this case, we’re actually supporting <Output>, which currently doesn’t have a container simply because it hasn’t needed one*. But here it looks like that is no longer the case.

So, I think we should introduce an Output container, and have the drag handling flow through it to the Redux store.

* Incidentally, there are a couple of other bits of logic that would benefit from an Output container—for instance, the question of whether to display the <Console> at all fits more naturally in <Output> rather than <Console> itself, but was more convenient to deal with in <Console> since the latter is a connected component. No need to make those changes as part of this PR but making an Output container would afford some additional code improvements.

Yes! I can go ahead and containerize the Output.

…raggable-console

pwjablonski · 2017-12-29T05:17:06Z

@outoftime took another pass at this with your suggestions. Still have some work to do but wanted some feedback on a couple of things.

Notes

I containerized both the output and editorColumn
I did not update to use classnames this time around. I will on the next go around.
I still am passing the entire flex array to the preview and output containers. Should I make specific selectors pick out the flex to avoid passing down the whole array?
I've been using environmentColumns and columns somewhat interchangeably. Also, just feedback on naming, in general, would be helpful.

Questions

Is this how we should structure initial state in the ui reducer?
Is it okay to have the selectors ie getResizableSectionRefs(state, 'output') pass the 'output' as a variable. Or do we need a separate selector for each section type? Same for getDividerRefs.
getDividerRefs returns an array even though in the current implementation there is only one divider per section. This is because I have not yet implemented it for the editors section which has multiple dividers. The reason I have not done this yet is that when using partial() within ref, the function gets called multiple times and ends up creating an infinite loop when updating the state. I think it is related to this. [BUG] ref function gets called twice on update (but not on first mount), first call with null value. facebook/react#9328. Thoughts on how to get around this?

outoftime · 2018-01-01T20:51:39Z

@pwjablonski

Is this how we should structure initial state in the ui reducer?

Here’s what I’d do:

Create a Record class to encapsulate the flex state of a particular draggable flex container (in general we should only use Map to represent arbitrary key/value pairs; if the keys are known in advance, Record is more appropriate, though following that rule across the entire codebase is a work in progress)
Have the initial state in the reducer just be an empty Map. Then the reducer can lazily initialize instances of the record as needed. This has the advantage of removing the knowledge of what specific draggable flex containers exist from the reducer altogether

Is it okay to have the selectors ie getResizableSectionRefs(state, 'output') pass the 'output' as a variable. Or do we need a separate selector for each section type? Same for getDividerRefs.

I’ve been wondering about this question myself, and it seems like the general consensus is that selectors should never take additional arguments besides the state. It looks like the general pattern is to expose a makeGetDividerRefs() higher-order function that returns a selector function—in fact we do this already in one other case.

getDividerRefs returns an array even though in the current implementation there is only one divider per section. This is because I have not yet implemented it for the editors section which has multiple dividers. The reason I have not done this yet is that when using partial() within ref, the function gets called multiple times and ends up creating an infinite loop when updating the state. I think it is related to this. facebook/react#9328. Thoughts on how to get around this?

Hmm I’m not sure I totally understand—might be easier for me to just look at the behavior in the wild—can you let me know how to reproduce it? Is it reproducible on the current version of this branch?

…raggable-console

pwjablonski · 2018-02-02T03:31:11Z

@outoftime finally got back to this.

My biggest question at the moment is regarding storing the dividerRefs and editorsRefs in the EditorColumn.jsx file. I tried to (poorly) explain this in my previous message.

In the previous implementation the ref was stored directly within the component and then consumed in the _handleEditorDividerDrag() function. See below.

this.editorRefs = [null, null, null];
.....

_storeEditorRef(index, ref) {
    this.editorRefs[index] = ref;
}
.....
   
dividerHeights: getNodeHeights(this.editorRefs),
.....

onRef={partial(this._stageEditorRef, index)}

This works as is, however, in this implementation the dividerRefs are never updated in the application state like they are for the other implementions (output and columns). To do this I orginally tried to just use the partial with the callback. This resulted in an infinite loop trying to update the state. (I believe is related to this facebook/react#9328)

_storeEditorRef(index, ref) {
    this.props.onStoreEditorRef(index, ref);
}
.....
onRef={partial(this._storeEditorRef, index)}

I ultimately got around this by making two functions. One to stage the changes within the component and then another to update the application state only when the component mounts.

  componentDidMount() {
    this._storeDividerRefs();
    this._storeEditorRefs();
  }

  _stageEditorRef(index, ref) {
    this.editorRefs[index] = ref;
  }

  _storeEditorRefs() {
    for (const [index, ref] of this.editorRefs.entries()) {
      this.props.onStoreEditorRef(index, ref);
    }
  }

Is there a better way to do this and is it even neccessary to update the application state

pwjablonski · 2018-02-02T03:47:59Z

Also updated the ui reducer. It now has records but I wasn't sure about where/how to lazily instantiate them.

outoftime · 2018-02-19T21:06:50Z

@pwjablonski OK, here to help finally!

So ultimately I think that the way refs are handled in the current live version of the code is correct—namely, they are stored directly in instance-level properties (not state in the React/Redux sense) on component instances. The component then adds information retrieved from the stored refs to the payload sent to the Redux store when dispatching resize actions.

From a structural standpoint, this makes sense because React components themselves should completely own the relationship between application state and the DOM. Ideally, no ref should ever leak out of the React component that describes the ref’s element; in practice, there is sometimes no great alternative to consuming a ref from a child component, but the surface area with the DOM is still confined entirely to the component hierarchy itself, and doesn’t leak out into application state (the Redux store).

From a practical standpoint, then, we should store the refs for resizable elements in the component that manages the resizing; in the case of the editors, that’s EditorsColumn, and in the case of the console, it’d be Output.

If we wanted to get really fancy, we could maybe create a higher-order component that dealt with the shared logic of storing refs / dispatching actions using those refs, but I’d probably do that as a refactor once we’ve got a basic thing working.

Let me know if that helps—happy to jump on a Google Hangout or talk it over at contributor night!

outoftime · 2018-02-22T02:34:04Z

src/components/Workspace.jsx

@@ -268,7 +188,7 @@ class Workspace extends React.Component {
            ref={this._storeDividerRef}
          />
        </DraggableCore>
-        {this._renderOutput()}
+        <Output />


<Output onRef={this._handleOutputRef} />

onRef is an “own prop” that is passed through to the Output presentational component.

pwjablonski · 2018-06-13T19:33:29Z

@outoftime can we close this down given the pending changes to draggable dividers?

outoftime · 2018-06-13T19:44:11Z

@pwjablonski yep, feel free to close!

pwjablonski added 3 commits December 22, 2017 10:12

first commit

8cb73e3

add draggable to output

0843ea7

fix merge conflicts

943f527

pwjablonski commented Dec 24, 2017

View reviewed changes

src/components/Console.jsx Outdated

}

// if (!isEnabled) {

// return null;

// }

Copy link

Contributor Author

pwjablonski Dec 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to uncomment this!

pwjablonski commented Dec 24, 2017

View reviewed changes

outoftime requested changes Dec 24, 2017

View reviewed changes

pwjablonski added 6 commits December 24, 2017 15:23

merge changes and fix conflicts

af9554c

containerize output

39d117a

generalize data flow for dragging

00bd3de

containerize editor column

ebb0a81

create selectors and update tests

3753675

Merge branch 'master' of https://github.com/popcodeorg/popcode into d…

c733535

…raggable-console

pwjablonski added 4 commits January 29, 2018 17:58

merge changes

2c3ecc2

update editorColumn to use redux to store editor and divider flex

ec4f95c

Merge branch 'master' of https://github.com/popcodeorg/popcode into d…

f5b4389

…raggable-console

pull changes and delete vscode file

13ddd99

outoftime reviewed Feb 22, 2018

View reviewed changes

pwjablonski closed this Jul 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draggable console #1310

Draggable console #1310

pwjablonski commented Dec 24, 2017

pwjablonski Dec 24, 2017

pwjablonski Dec 24, 2017

outoftime Dec 24, 2017

pwjablonski Dec 24, 2017

outoftime left a comment

outoftime Dec 24, 2017

pwjablonski Dec 27, 2017

outoftime Dec 24, 2017

pwjablonski Dec 27, 2017

outoftime Dec 24, 2017

pwjablonski Dec 27, 2017

pwjablonski commented Dec 29, 2017 •

edited

Loading

outoftime commented Jan 1, 2018

pwjablonski commented Feb 2, 2018 •

edited

Loading

pwjablonski commented Feb 2, 2018

outoftime commented Feb 19, 2018 •

edited

Loading

outoftime Feb 22, 2018

pwjablonski commented Jun 13, 2018

outoftime commented Jun 13, 2018

Draggable console #1310

Draggable console #1310

Conversation

pwjablonski commented Dec 24, 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

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

pwjablonski commented Dec 29, 2017 • edited Loading

outoftime commented Jan 1, 2018

pwjablonski commented Feb 2, 2018 • edited Loading

pwjablonski commented Feb 2, 2018

outoftime commented Feb 19, 2018 • edited Loading

Choose a reason for hiding this comment

pwjablonski commented Jun 13, 2018

outoftime commented Jun 13, 2018

pwjablonski commented Dec 29, 2017 •

edited

Loading

pwjablonski commented Feb 2, 2018 •

edited

Loading

outoftime commented Feb 19, 2018 •

edited

Loading