Focus on expanded editor #1086

YiningChen · 2017-10-09T01:34:40Z

Focuses editor when user expands a collapsed editor! (Issue #1080)

to allow for extra data to be stored for each component in hiddenUIComponents, I turned it from a Set to a Map, with the key being referenced as componentKey in parameters. (before it was sometimes called componentName and other times just component)
Also, for the language editors, I just used the language as the key instead of editor.${language}, as "editor" is stored as componentType for each hidden component.
and created a migration function within records/Project so that old snapshots and gists will still work seamlessly! (But I'm not sure if that's the correct place for such a function xD I mainly did so to avoid a circular file reference, as any migration function would need to import the HiddenUIComponent record)

Also, as I was working I was wondering if instead of hiddenUIComponents in state, it could just be UIComponents with a hidden property. It might also make it easier to tell if new components like the console and instructions were purposefully expanded, or if they just didn't exist for old projects.

YiningChen · 2017-10-09T01:43:12Z

src/reducers/ui.js

+
+function restoreFlexOnComponentToggle(componentName, state) {
+  if (componentName === 'output') {
+    return restoreDefaultRowFlex(state);


It doesn't seem like the output component is ever hidden, but left this in here in case that's part of the roadmap. Happy to delete this functionality and simplify if that would be better!

Yep, you’re right about that—it actually used to be possible, but the redesign didn’t have an output-minimization button, so I went ahead and dropped it from the initial implementation of the new layout. I think we could probably just put one in the preview top bar (next to either the refresh or the pop-out button) but also no one has mentioned missing it…

outoftime

@YiningChen this is great! I am always happy to be brutal in code review but nothing to be brutal about here : )

Left a couple of minor style comments and raised one more substantive issue, but overall the approach here looks exactly right and is very nicely implemented!

outoftime · 2017-10-09T01:38:43Z

src/reducers/ui.js

@@ -24,6 +24,29 @@ const defaultState = new Immutable.Map().
  set('topBar', new Immutable.Map({openMenu: null})).
  set('lastRefreshTimestamp', null);

+// i.e. restore to before editors were resized


I’m generally not a fan of most “explanatory” comments—it’s very easy for them to diverge from the code they seek to explain, and if variables/functions/classes are named carefully, I don’t think comments are usually necessary. In this case I think the function names restoreDefaultColumnFlex etc. are entirely self-documenting!

outoftime · 2017-10-09T01:41:48Z

src/reducers/ui.js

-      }
-      return state.setIn(['workspace', 'columnFlex'], DEFAULT_COLUMN_FLEX);
+      state = focusEditor(
+        action.payload.componentName.replace('editor.', ''),


This is definitely the most low-impact way to do what we need to do here, but I’m not in love with the idea of parsing a string that’s just being used as an internal identifier. Seems like it might be better to change the structure of the action a bit so that the information we need is discretely available. Maybe something like {componentType: 'editor', componentId: 'javascript'}?

Okay! That makes a lot of sense! Thanks :)

outoftime · 2017-10-09T01:42:03Z

src/reducers/ui.js

-      return state.setIn(['workspace', 'columnFlex'], DEFAULT_COLUMN_FLEX);
+      state = focusEditor(
+        action.payload.componentName.replace('editor.', ''),
+        state);


Very minor nitpick, but project style would be to put the ) on the next line : )

outoftime · 2017-10-26T00:50:02Z

heyyy @YiningChen any idea when we might be able to get this puppy over the finish line?? it’s so close!

YiningChen · 2017-10-26T00:52:44Z

ah!! Sorry @outoftime had a busy couple of weeks 😱, but planning on picking it back up tomorrow!

outoftime · 2017-10-26T01:11:37Z

@YiningChen no worries, totally understand! can’t wait to ship it!

YiningChen · 2017-10-30T02:03:53Z

src/reducers/projects.js

@@ -88,6 +92,7 @@ export function reduceRoot(stateIn, action) {

 export default function reduceProjects(stateIn, action) {
  let state;
+  const {payload} = action;


I did this to get past the line length linter 😂 But let me know if this would cause issues!

I have a policy of not really nitpicking the reducers until after I am able to refactor them to have a reasonable structure.

I would actually say this is a good change on the merits—in fact, I’d probably argue for just fully destructuring action in the params list itself!

Ah! Right that totally makes sense! thanks!

outoftime · 2017-10-31T00:50:59Z

src/util/componentName.js

+  return componentType;
+}
+
+


Couple extra lines of whitespace here

outoftime · 2017-10-31T00:54:43Z

src/util/componentName.js

@@ -0,0 +1,8 @@
+export function makeComponentName({componentType, componentId}) {


@YiningChen can you share the reasoning for having this? In particular, why not just make the {componentType, componentId} structure the thing we use universally to describe a UI component, rather than still using the string representation in some cases?

No good reasoning! I assumed there was already a reason it was a string, which is a bad reason xD Will change, thanks!

outoftime · 2017-10-31T00:55:27Z

src/reducers/projects.js

@@ -88,6 +92,7 @@ export function reduceRoot(stateIn, action) {

 export default function reduceProjects(stateIn, action) {
  let state;
+  const {payload} = action;


I would actually say this is a good change on the merits—in fact, I’d probably argue for just fully destructuring action in the params list itself!

outoftime · 2017-10-31T00:56:19Z

@YiningChen getting verrrrrrry close i think! sorry about the annoying component structure thing, that gives this a much bigger footprint than it would have otherwise, but i still think it’s good!

YiningChen · 2017-10-31T18:31:39Z

No worries! It's been fun digging into more and more as well! :D

outoftime · 2017-11-18T20:44:53Z

@YiningChen would love to ship this! it’s very close! now that the Britney Pickle Spears have been served, think you might have a chance to bring it home?

YiningChen · 2017-11-20T18:27:00Z

lol! I have a working version that refactored every call to show/hide actions, but ran into a weird bug that I think was related to a previous saved project's schema not matching up to the new schema (where hiddenUIComponents is an array of objects instead of a set of strings). Seemed to work after forcing it to be an array with lodash's concat, but wanted to play around with it a bit more to see if there's a better solution!

outoftime · 2017-11-21T02:24:38Z

@YiningChen gotcha! i figured that kind of issue would come up sooner or later (actually surprised it wasn’t sooner)—I think the move is probably to introduce the concept of “migrations” that would be applied to projects right when we dehydrate them from Firebase. Basically the idea I had in mind was to create a collection of functions, each of which takes a project, checks it for a particular outdated structure (in this case, strings for hidden components), and then “migrates” it to the newer structure. As we make more changes to how projects are structured, we’d just add more functions to the list. Does that seem reasonable?

YiningChen · 2017-11-21T02:28:37Z

Oooo okay! yeah definitely! 😄 I'll aim to get that done latest soon after thanksgiving! :)

… is happening

…chema

outoftime · 2018-03-27T01:54:29Z

@YiningChen exciting! I think I’ll probably have time to really dig into this Wednesday but just perusing it a bit, I think it would be really helpful if you could update the PR description with a high-level outline of the code changes here—in particular I’m not quite grasping the idea of the “hidden component line” data structure and how it fits into the overall change we’re making here. Can’t wait to dive in deeper!

outoftime · 2018-03-27T01:54:57Z

(Also it looks like there’s a conflict with master right now so would be great to resolve that if you have a chance)

YiningChen · 2018-03-27T02:38:16Z

Ah yes! sorry! will do!

YiningChen · 2018-03-27T03:14:46Z

src/components/Editor.jsx

@@ -58,13 +58,15 @@ class Editor extends React.Component {
  }

  componentWillUnmount() {
+    const {onComponentHidden, language} = this.props;
+    const {row, column} = this._editor.getCursorPosition();
+    onComponentHidden(language, row, column);


Not sure if there is a better place for this action to be called, I would have ideally liked to call it from EditorContainer.jsx, where the hide function is called.

outoftime · 2018-03-27T12:38:09Z

@YiningChen great writeup! looking forward to digging into this 😀

outoftime

@YiningChen OK! I’ve just gone through and gotten good and familiar with the changes here.

Holding off on line-level feedback for now as I wanted to bring up one high-level thought. As I understand it, the basic sequence of events that happens when you hide an editor component is:

Action dispatched to Redux requesting that editor be hidden
Redux updates store to mark the editor as hidden
React components update, causing editor component to unmount
componentWillUnmount lifecycle hook on editor component dispatches an action to Redux declaring, with its last breath, “this is the row and column that was focused”
Redux store handles this action, annotating the “hidden component” object with the row/column information
When it’s time to show the editor component again, the UI dispatches a focusLine action which includes the row/column metadata stored in step 5

Assuming I’m not totally off-base here, I am a little uneasy about the above, for a handful of related reasons:

It feels weird to dispatch a Redux action from a componentWillUnmount lifecycle hook. In general I expect the UI (React component hierarchy) to change in response to changes in the Redux store, not vice versa.
More simply, any time we are dispatching multiple actions to fully handle a single user action, it’s a sign that something may be amiss. We should be able to dispatch a single action that says “the user did X” and the reducers/sagas should be able to make updates accordingly.
The HiddenComponent record type has line and column properties, which are only meaningful for one special case of hidden component.
As a corollary to the multiple-actions observation above, actions should also straightforwardly describe what happened—in this case we are using an action that encodes “the user asked for this line to be focused”, but in reality the user simply asked for the component to be displayed. This may be a simple matter of naming though.

Taking a step back, I would propose that we’re really dealing with two distinct pieces of information:

What components are currently visible?
What is the user’s cursor position within each source in the current project?

Looking at it that way, I think the most straightforward way of modeling that information would be to store the cursor positions in their own place in the ui state, updating it every time the user moves the cursor or changes the selection. This may seem crazy but we already update the state every time the user presses a key, so it’s safe to say there are no rules.

Under this regime the <Editor> component would be responsible not only for updating the cursor position in the Redux store, but also for consuming that information and updating the Ace editor’s internal cursor position to match received props.

One advantage of this proposal is that it would obviate the rather tortured requestedFocusedLine approach that has always felt like a bit of a kludge—a click on an error, for instance, would straightforwardly update UI state saying “this is where the cursor should be now”.

That said: the whole idea of restoring the exact cursor position when a hidden element is un-hidden is a lovely UX flourish but feels like something that could absolutely happen in a followup pull request. I’m more than happy to work with you on finding the best possible way to do that in the context of this PR, but I am also totally down to punt on it in service of getting the core of this enhancement released sooner. There’s a lot to like here!

I am also, as always, 100% down to be convinced that everything I’ve wrote here is misguided and the current approach is in fact the best solution for the problem!!

Let me know your thoughts!

YiningChen · 2018-03-29T02:53:45Z

Hi Matt!

That definitely makes a lot sense, and it didn't feel 100% right when I called that action from componentWillUnmount so I'm definitely in agreement! And storing line/column position definitely doesn't seem worth it just to restore position when an component is unhidden. Happy to dial back the implementation! How do you feel about the HiddenUIComponent record with line and column removed? I originally went down this route because we discussed having a componentType and language property for the editors, which I do really like having for situations like this: https://github.com/popcodeorg/popcode/pull/1086/files#diff-cf58228d41d0dc144163bbeb96dbe489R65

Also out of curiosity, how come the editors are destroyed and recreated when toggled as opposed to using CSS to hide them? I would love to follow up this in the future, and the best implementation I can think of to avoid storing line/column would to just refocus the editor using focus().

Thanks for the thorough review! :)

outoftime · 2018-03-29T11:35:19Z

@YiningChen great question/thought. Off the top of my head, I can’t recall any specific intent behind adding/removing the editors from the DOM vs. just hiding them with CSS. The latter certainly seems worth exploring!

…ch renaming of onEditorHide

…her functions passed from props

YiningChen · 2018-04-06T01:10:20Z

src/components/EditorsColumn.jsx

-        currentProject.hiddenUIComponents,
-        `editor.${language}`,
-      ),
+      language => find(currentProject.hiddenUIComponents, {language}),


@outoftime My only hesitation with renaming the language property in HiddenUIComponent to instance is this line, since I really like the how the language property connects to how language is used in this file. i.e. language is more specific about the piece of information it's storing. instance also wouldn't be used by anything that isn't an editor, and I can't imagine we'd want multiple consoles or instructions.

I feel you, but I think the decoupling of the property from the specific use case here is valuable enough to make it worth it. And I think passing the search spec {instance: language} here would actually be nicely self-documenting regarding the relationship between the two.

YiningChen · 2018-04-26T13:24:50Z

src/components/Editor.jsx

@@ -63,8 +63,7 @@ class Editor extends React.Component {
  }

  _focusRequestedLine(requestedFocusedLine) {
-    if (get(requestedFocusedLine, 'component') !==
-      `editor.${this.props.language}`) {
+    if (get(requestedFocusedLine, 'componentKey') !== this.props.language) {


it doesn't feel 100% right here to use language as the componentKey, but I wanted to get rid of all editor.${this.props.language}, which also feels a bit arbitrary. Please let me know if you have better ideas!

So it seems like the structure of HiddenUIComponent could do the work here too—basically this is another case where we are referring to a specific UI component instance in the context of the Redux store. That would argue for giving HiddenUIComponent a more general name, something like UIComponentInstance (or something more poetic), but I think that would be a good change anyway since there’s nothing about that record that is tied to the concern of hiding/showing. WDYT?

(The same observation would of course apply here as well.)

YiningChen · 2018-04-26T15:45:57Z

ready for review again!

outoftime · 2018-04-27T23:32:25Z

@YiningChen awesome! will look at this as soon as i can!

outoftime

@YiningChen hey this is looking really good!! left a bunch of comments but largely driving at one high-level thing which is that I think there is enough overlap between the concept of a componentKey and a HiddenUIComponent record that we should consolidate them (and prefer the structure of HiddenUIComponent, though probably with a more generalized name).

Let me know if what I’ve written here makes sense and whether you agree!

outoftime · 2018-05-02T05:39:30Z

src/actions/projects.js

 );

 export const toggleComponent = createAction(
  'TOGGLE_COMPONENT',
-  (projectKey, componentName) => ({projectKey, componentName}),
-  (_projectKey, _componentName, timestamp = Date.now()) => ({timestamp}),
+  (projectKey, componentKey, hiddenUIComponent) =>


I think the hiddenUIComponent parameter here is a vestige of an earlier approach?

outoftime · 2018-05-02T05:40:50Z

src/clients/github.js

@@ -106,7 +106,8 @@ function buildGistFromProject(project) {
      language: 'Markdown',
    };
  }
-  if (project.enabledLibraries.length || project.hiddenUIComponents.length) {
+  if (project.enabledLibraries.length ||


I like the change to isEmpty on the next line, let’s do it here too?

outoftime · 2018-05-02T08:32:12Z

src/components/Editor.jsx

@@ -63,8 +63,7 @@ class Editor extends React.Component {
  }

  _focusRequestedLine(requestedFocusedLine) {
-    if (get(requestedFocusedLine, 'component') !==
-      `editor.${this.props.language}`) {
+    if (get(requestedFocusedLine, 'componentKey') !== this.props.language) {


So it seems like the structure of HiddenUIComponent could do the work here too—basically this is another case where we are referring to a specific UI component instance in the context of the Redux store. That would argue for giving HiddenUIComponent a more general name, something like UIComponentInstance (or something more poetic), but I think that would be a good change anyway since there’s nothing about that record that is tied to the concern of hiding/showing. WDYT?

outoftime · 2018-05-02T08:34:13Z

src/components/Editor.jsx

@@ -63,8 +63,7 @@ class Editor extends React.Component {
  }

  _focusRequestedLine(requestedFocusedLine) {
-    if (get(requestedFocusedLine, 'component') !==
-      `editor.${this.props.language}`) {
+    if (get(requestedFocusedLine, 'componentKey') !== this.props.language) {


(The same observation would of course apply here as well.)

outoftime · 2018-05-02T08:39:25Z

src/components/EditorsColumn.jsx

-        currentProject.hiddenUIComponents,
-        `editor.${language}`,
-      ),
+      language => find(currentProject.hiddenUIComponents, {language}),


I feel you, but I think the decoupling of the property from the specific use case here is valuable enough to make it worth it. And I think passing the search spec {instance: language} here would actually be nicely self-documenting regarding the relationship between the two.

outoftime · 2018-05-02T09:06:18Z

src/components/Workspace.jsx

    this.props.dispatch(
      hideComponent(
        this.props.currentProject.projectKey,
-        componentName,
+        language,
+        new HiddenUIComponent({componentType: 'editor', language}),


Building on some of my previous feedback, it seems like we’re duplicating a bit of information here (specifically language) and could broadly be more consistent about how we’re referencing UI components. In particular, I think the properties of HiddenUIComponent—a componentType and optional instance—comprise the right way to describe a UI component in all situations that we need to:

Hide/unhide component

Request focused line

Maybe others I am not thinking of?

So my proposal would be that action creators for the above always take a componentType and optional instance, and the resulting structures in state always use a HiddenUIComponent record. WDYT?

outoftime · 2018-05-02T11:43:09Z

src/records/Project.js

@@ -12,23 +31,25 @@ export default class Project extends Record({
  projectKey: null,
  sources: new Sources(),
  enabledLibraries: new Set(),
-  hiddenUIComponents: new Set(['console']),
+  hiddenUIComponents: new Map({
+    console: new HiddenUIComponent({componentType: 'console'}),


So the more I look at this the more I think that the componentKey idea isn’t actually getting us anything, and is just a sort of lossy alternate representation of the information in the HiddenUIComponent record. My pitch would be to get rid of the componentKey altogether and always just refer to components using the collection of properties contained in HiddenUIComponent.

In order to do what we want to do here, I would suggest making this a Map where the keys are HiddenUIComponent records, and the values are booleans, with true meaning the component should be hidden, and false meaning it should be visible. Or we could rename it to something like componentVisibility and invert the meaning of the booleans. In the case where a map key is not present at all, we’d take that as “do the default” for that component (currently, show everything except the console).

outoftime · 2018-05-02T11:49:55Z

src/records/HiddenUIComponent.js

+import {Record} from 'immutable';
+
+export default Record({
+  componentType: '',


I would probably just make this default to null too?

outoftime · 2018-05-02T11:51:03Z

src/records/Project.js

+    obj = hiddenUIComponents;
+  } else if (isArray(hiddenUIComponents)) {
+    let component;
+    for (component of hiddenUIComponents) {


How about for (const component of hiddenUIComponents) ?

outoftime · 2018-05-02T11:51:53Z

src/reducers/projects.js

-      components => components.add('console'),
-    ),
-  );
+  return state.set(project.projectKey, Project.fromJS(project));


outoftime · 2018-08-11T19:33:44Z

@YiningChen any chance you have the bandwidth to pick this up and bring it to the finish line? Happy to help out with merge conflicts or if you’re too busy, I can take over, just let me know!

YiningChen · 2018-08-14T05:59:13Z

ah sorry I totally forgot about this for awhile! started a new job and moved across the country xD I should have time this weekend! but if not totally fine with it being taken over :) Thanks!

outoftime · 2018-08-15T01:17:01Z

Oh wow congrats! No wild rush at all, just wanted to see if you were up for finishing it : )

yining and others added 3 commits October 8, 2017 20:54

move hide/unhide reducer logic into outside functions

8bb5b1e

focus editor when unhidden

57acd97

Merge branch 'master' into 1080-focus-on-expanded-editor

49adda4

YiningChen commented Oct 9, 2017

View reviewed changes

outoftime requested changes Oct 9, 2017

View reviewed changes

YiningChen and others added 2 commits October 11, 2017 16:26

Merge branch 'master' into 1080-focus-on-expanded-editor

a75a68e

code review: code style changes

95be55b

yining added 2 commits October 29, 2017 21:59

make helper to generate component name

17d376f

change "componentName" structure in state schema

8c82569

YiningChen commented Oct 30, 2017

View reviewed changes

yining added 3 commits October 29, 2017 22:11

merge in master & fix conflict in reducers/projects.js

1b9ea4d

update reducers tests to match changes

dcf0d5b

add help for installing older version of yarn in README

71e7453

outoftime requested changes Oct 31, 2017

View reviewed changes

Yining Chen added 7 commits March 20, 2018 21:11

destructure action properties in params, less typing!

f139ff0

move functionality to function with name to help make more clear what…

f35b419

… is happening

add record for hidden ui component

a7727fd

use new HiddenUIComponent record in project record

8e5d1aa

change reducers and components to use new hiddenUIComponents object s…

b7e917d

…chema

remove commented out code

5b570dd

simplify and consolidate logic between components and reducers a bit

016ed36

Yining Chen added 2 commits March 26, 2018 12:47

delete old functionality

af3efd5

match new destructing of action object in params for new reducers

4b310e9

Yining Chen added 2 commits March 26, 2018 22:43

merge in master

cdfd758

match meta function param names with payload function

20bfc52

YiningChen commented Mar 27, 2018

View reviewed changes

outoftime reviewed Mar 29, 2018

View reviewed changes

Yining Chen and others added 7 commits April 5, 2018 19:46

remove functionality to restore saved editor position

1129545

rename onComponentUnhide to onEditorUnhide for specificity and to mat…

a3d519b

…ch renaming of onEditorHide

reimplement focusing editor

848ef59

bring back unhide & hide reducer tests and update to match changes

30369fe

Merge branch 'master' into 1080-focus-on-expanded-editor

4f05a4b

assign onEditorUnhide instead of using it from props, to match all ot…

23315cb

…her functions passed from props

missed a reducer I was supposed to delete

c447995

YiningChen commented Apr 6, 2018

View reviewed changes

YiningChen commented Apr 26, 2018

View reviewed changes

YiningChen and others added 2 commits April 26, 2018 09:25

Merge branch 'master' into 1080-focus-on-expanded-editor

ab67c9c

update code from master to use destructing of action props

f1fdd7d

outoftime requested changes May 2, 2018

View reviewed changes

outoftime changed the title ~~1080 focus on expanded editor~~ Focus on expanded editor Aug 15, 2018

		@@ -0,0 +1,8 @@
		export function makeComponentName({componentType, componentId}) {

Focus on expanded editor #1086

Are you sure you want to change the base?

Focus on expanded editor #1086

Conversation

YiningChen commented Oct 9, 2017 • edited Loading

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

outoftime commented Oct 26, 2017

YiningChen commented Oct 26, 2017

outoftime commented Oct 26, 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

outoftime commented Oct 31, 2017

YiningChen commented Oct 31, 2017

outoftime commented Nov 18, 2017

YiningChen commented Nov 20, 2017

outoftime commented Nov 21, 2017

YiningChen commented Nov 21, 2017

outoftime commented Mar 27, 2018

outoftime commented Mar 27, 2018

YiningChen commented Mar 27, 2018

Choose a reason for hiding this comment

outoftime commented Mar 27, 2018

outoftime left a comment • edited Loading

Choose a reason for hiding this comment

YiningChen commented Mar 29, 2018

outoftime commented Mar 29, 2018

YiningChen Apr 6, 2018 • edited Loading

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

YiningChen commented Apr 26, 2018

outoftime commented Apr 27, 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

outoftime commented Aug 11, 2018

YiningChen commented Aug 14, 2018

outoftime commented Aug 15, 2018

YiningChen commented Oct 9, 2017 •

edited

Loading

outoftime left a comment •

edited

Loading

YiningChen Apr 6, 2018 •

edited

Loading