Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Focus on expanded editor #1086

Open
wants to merge 42 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
8bb5b1e
move hide/unhide reducer logic into outside functions
Oct 9, 2017
57acd97
focus editor when unhidden
Oct 9, 2017
49adda4
Merge branch 'master' into 1080-focus-on-expanded-editor
YiningChen Oct 9, 2017
a75a68e
Merge branch 'master' into 1080-focus-on-expanded-editor
YiningChen Oct 11, 2017
95be55b
code review: code style changes
Oct 11, 2017
17d376f
make helper to generate component name
Oct 30, 2017
8c82569
change "componentName" structure in state schema
Oct 30, 2017
1b9ea4d
merge in master & fix conflict in reducers/projects.js
Oct 30, 2017
dcf0d5b
update reducers tests to match changes
Oct 30, 2017
71e7453
add help for installing older version of yarn in README
Oct 30, 2017
f139ff0
destructure action properties in params, less typing!
Mar 21, 2018
f35b419
move functionality to function with name to help make more clear what…
Mar 21, 2018
a7727fd
add record for hidden ui component
Mar 21, 2018
8e5d1aa
use new HiddenUIComponent record in project record
Mar 21, 2018
b7e917d
change reducers and components to use new hiddenUIComponents object s…
Mar 21, 2018
5b570dd
remove commented out code
Mar 21, 2018
016ed36
simplify and consolidate logic between components and reducers a bit
Mar 22, 2018
9708c5e
add new action and reducer to store hidden line data
Mar 23, 2018
0bf16b6
standardize component reference to "componentKey"
Mar 23, 2018
cbc1847
focus saved line when unhiding!
Mar 23, 2018
c86dc08
use isEmpty() instead of .length because hiddenUIComponents is now an…
Mar 24, 2018
08aaf85
migrate stored JSON before converting to Map
Mar 24, 2018
11e6aaf
merge in master
Mar 24, 2018
98bc1ac
use `hiddenUIComponents` from `currentProject`
Mar 24, 2018
7cc3bdf
take advantage of `language` property in hiddenUIComponents for clear…
Mar 24, 2018
d14d894
use toggle action instead of hide/unhide, and change parameters to us…
Mar 25, 2018
2fe7483
move migration function into records
Mar 25, 2018
6e92d65
update tests to reflect changes
Mar 25, 2018
22760ab
Merge branch '1080-focus-expanded-editor-take-2-yining' into 1080-foc…
Mar 26, 2018
af3efd5
delete old functionality
Mar 26, 2018
4b310e9
match new destructing of `action` object in params for new reducers
Mar 26, 2018
cdfd758
merge in master
Mar 27, 2018
20bfc52
match meta function param names with payload function
Mar 27, 2018
1129545
remove functionality to restore saved editor position
Apr 5, 2018
a3d519b
rename onComponentUnhide to onEditorUnhide for specificity and to mat…
Apr 5, 2018
848ef59
reimplement focusing editor
Apr 5, 2018
30369fe
bring back unhide & hide reducer tests and update to match changes
Apr 6, 2018
4f05a4b
Merge branch 'master' into 1080-focus-on-expanded-editor
YiningChen Apr 6, 2018
23315cb
assign onEditorUnhide instead of using it from props, to match all ot…
Apr 6, 2018
c447995
missed a reducer I was supposed to delete
Apr 6, 2018
ab67c9c
Merge branch 'master' into 1080-focus-on-expanded-editor
YiningChen Apr 26, 2018
f1fdd7d
update code from master to use destructing of `action` props
Apr 26, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,8 @@ When you're done, lint and make sure tests pass before opening a pull request:
$ yarn test
```

If you're using homebrew and the latest version of yarn is incompatible with the current one used by popcode, you can check out this [stack overflow to install an older version of yarn](https://stackoverflow.com/questions/39187812/homebrew-how-to-install-older-versions)

### Debug Mode ###

By default, Popcode’s JavaScript code is compiled to ES5 to support a wide
Expand Down
6 changes: 2 additions & 4 deletions src/actions/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@ import {
createProject,
changeCurrentProject,
toggleLibrary,
hideComponent,
unhideComponent,
storeHiddenComponentLine,
toggleComponent,
updateProjectSource,
updateProjectInstructions,
Expand Down Expand Up @@ -75,8 +74,7 @@ export {
userAuthenticated,
userLoggedOut,
addRuntimeError,
hideComponent,
unhideComponent,
storeHiddenComponentLine,
toggleComponent,
focusLine,
editorFocusedRequestedLine,
Expand Down
20 changes: 8 additions & 12 deletions src/actions/projects.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,22 +30,18 @@ export const toggleLibrary = createAction(
(_projectKey, _libraryKey, timestamp = Date.now()) => ({timestamp}),
);

export const hideComponent = createAction(
'HIDE_COMPONENT',
(projectKey, componentName) => ({projectKey, componentName}),
(_projectKey, _componentName, timestamp = Date.now()) => ({timestamp}),
);

export const unhideComponent = createAction(
'UNHIDE_COMPONENT',
(projectKey, componentName) => ({projectKey, componentName}),
(_projectKey, _componentName, timestamp = Date.now()) => ({timestamp}),
export const storeHiddenComponentLine = createAction(
'STORE_HIDDEN_LINE',
(projectKey, componentKey, line, column) =>
({projectKey, componentKey, line, column}),
);

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

Choose a reason for hiding this comment

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

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

({projectKey, componentKey, hiddenUIComponent}),
(projectKey, componentKey, hiddenUIComponent, timestamp = Date.now()) =>
({timestamp}),
);

export const gistImported = createAction(
Expand Down
4 changes: 2 additions & 2 deletions src/actions/ui.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@ export const userDoneTyping = createAction('USER_DONE_TYPING');

export const focusLine = createAction(
'FOCUS_LINE',
(component, line, column) => ({component, line, column}),
(_component, _line, _column, timestamp = Date.now()) => ({timestamp}),
(componentKey, line, column) => ({componentKey, line, column}),
(_componentKey, _line, _column, timestamp = Date.now()) => ({timestamp}),
);

export const editorFocusedRequestedLine = createAction(
Expand Down
5 changes: 3 additions & 2 deletions src/clients/github.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,8 @@ function buildGistFromProject(project) {
language: 'Markdown',
};
}
if (project.enabledLibraries.length || project.hiddenUIComponents.length) {
if (project.enabledLibraries.length ||
Copy link
Contributor

Choose a reason for hiding this comment

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

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

!isEmpty(project.hiddenUIComponents)) {
files['popcode.json'] = {
content: createPopcodeJson(project),
language: 'JSON',
Expand Down Expand Up @@ -137,7 +138,7 @@ function createPopcodeJson(project) {
if (project.enabledLibraries.length) {
json.enabledLibraries = project.enabledLibraries;
}
if (project.hiddenUIComponents.length) {
if (!isEmpty(project.hiddenUIComponents)) {
json.hiddenUIComponents = project.hiddenUIComponents;
}
return JSON.stringify(json);
Expand Down
2 changes: 1 addition & 1 deletion src/components/ConsoleInput.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export default class ConsoleInput extends Component {
}

_focusRequestedLine(requestedFocusedLine) {
if (get(requestedFocusedLine, 'component') !== 'console') {
if (get(requestedFocusedLine, 'componentKey') !== 'console') {
return;
}

Expand Down
7 changes: 5 additions & 2 deletions src/components/Editor.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

this._editor.destroy();
window.removeEventListener('resize', this._handleWindowResize);
}

_focusRequestedLine(requestedFocusedLine) {
if (get(requestedFocusedLine, 'component') !==
`editor.${this.props.language}`) {
if (get(requestedFocusedLine, 'componentKey') !== this.props.language) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

return;
}

Expand Down Expand Up @@ -141,6 +143,7 @@ Editor.propTypes = {
requestedFocusedLine: PropTypes.object,
source: PropTypes.string.isRequired,
textSizeIsLarge: PropTypes.bool.isRequired,
onComponentHidden: PropTypes.func.isRequired,
onInput: PropTypes.func.isRequired,
onRequestedLineFocused: PropTypes.func.isRequired,
};
Expand Down
14 changes: 7 additions & 7 deletions src/components/EditorsColumn.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ import {t} from 'i18next';
import {DraggableCore} from 'react-draggable';
import bindAll from 'lodash/bindAll';
import isEmpty from 'lodash/isEmpty';
import includes from 'lodash/includes';
import partial from 'lodash/partial';
import partition from 'lodash/partition';
import find from 'lodash/find';
import {getNodeHeights} from '../util/resize';

import EditorContainer from './EditorContainer';
Expand Down Expand Up @@ -50,6 +50,7 @@ export default class EditorsColumn extends React.Component {
currentProject,
editorsFlex,
errors,
onComponentHidden,
onComponentHide,
onEditorInput,
onRef,
Expand All @@ -61,10 +62,7 @@ export default class EditorsColumn extends React.Component {
const children = [];
const [hiddenLanguages, visibleLanguages] = partition(
['html', 'css', 'javascript'],
language => includes(
currentProject.hiddenUIComponents,
`editor.${language}`,
),
language => find(currentProject.hiddenUIComponents, {language}),
Copy link
Collaborator Author

@YiningChen YiningChen Apr 6, 2018

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

);
visibleLanguages.forEach((language, index) => {
children.push(
Expand All @@ -73,7 +71,7 @@ export default class EditorsColumn extends React.Component {
language={language}
source={currentProject.sources[language]}
style={{flex: editorsFlex[index]}}
onHide={partial(onComponentHide, `editor.${language}`)}
onHide={partial(onComponentHide, language)}
onRef={partial(this._storeEditorRef, index)}
>
<Editor
Expand All @@ -84,6 +82,7 @@ export default class EditorsColumn extends React.Component {
requestedFocusedLine={ui.editors.requestedFocusedLine}
source={currentProject.sources[language]}
textSizeIsLarge={ui.editors.textSizeIsLarge}
onComponentHidden={onComponentHidden}
onInput={partial(onEditorInput, language)}
onRequestedLineFocused={onRequestedLineFocused}
/>
Expand Down Expand Up @@ -111,7 +110,7 @@ export default class EditorsColumn extends React.Component {
key={language}
onClick={partial(
this.props.onComponentUnhide,
`editor.${language}`,
language,
)}
>
<div className="label editors__label editors__label_collapsed">
Expand Down Expand Up @@ -147,6 +146,7 @@ EditorsColumn.propTypes = {
ui: PropTypes.shape({
editors: PropTypes.object.isRequired,
}).isRequired,
onComponentHidden: PropTypes.func.isRequired,
onComponentHide: PropTypes.func.isRequired,
onComponentUnhide: PropTypes.func.isRequired,
onDividerDrag: PropTypes.func.isRequired,
Expand Down
29 changes: 18 additions & 11 deletions src/components/Workspace.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,20 +17,20 @@ import {dehydrateProject, rehydrateProject} from '../clients/localStorage';

import {
updateProjectSource,
hideComponent,
unhideComponent,
editorFocusedRequestedLine,
dragRowDivider,
dragColumnDivider,
startDragColumnDivider,
stopDragColumnDivider,
toggleComponent,
applicationLoaded,

storeHiddenComponentLine,
focusLine,
} from '../actions';

import {isPristineProject} from '../util/projectUtils';
import {getCurrentProject, isEditingInstructions} from '../selectors';
import {HiddenUIComponent} from '../records';
Copy link
Contributor

Choose a reason for hiding this comment

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

I’m not a huge fan of having a React component explicitly depend on a Redux record class—it’s not something we’ve done elsewhere and increases coupling between the UI implementation and the application state implementation. Instead how about we just pass the componentType and language as discrete arguments to hideComponent and let the reducer handle constructing the record instance?


import TopBar from '../containers/TopBar';
import Instructions from '../containers/Instructions';
Expand Down Expand Up @@ -61,6 +61,7 @@ class Workspace extends React.Component {
this,
'_handleUnload',
'_handleClickInstructionsBar',
'_handleComponentHidden',
'_handleComponentUnhide',
'_handleComponentHide',
'_handleDividerDrag',
Expand Down Expand Up @@ -114,24 +115,29 @@ class Workspace extends React.Component {
}
}

_handleComponentHide(componentName) {
_handleComponentHidden(componentKey, line, column) {
const {projectKey} = this.props.currentProject;
this.props.dispatch(
hideComponent(
this.props.currentProject.projectKey,
componentName,
),
storeHiddenComponentLine(projectKey, componentKey, line, column),
);
}

_handleComponentUnhide(componentName) {
_handleComponentHide(language) {
this.props.dispatch(
unhideComponent(
toggleComponent(
this.props.currentProject.projectKey,
componentName,
language,
new HiddenUIComponent({componentType: 'editor', language}),
),
);
}

_handleComponentUnhide(componentKey) {
const {dispatch, currentProject} = this.props;
const {line, column} = currentProject.hiddenUIComponents[componentKey];
dispatch(focusLine(componentKey, line, column));
}

_handleEditorInput(language, source) {
this.props.dispatch(
updateProjectSource(
Expand Down Expand Up @@ -266,6 +272,7 @@ class Workspace extends React.Component {
errors={errors}
style={{flex: rowsFlex[0]}}
ui={ui}
onComponentHidden={this._handleComponentHidden}
onComponentHide={this._handleComponentHide}
onComponentUnhide={this._handleComponentUnhide}
onDividerDrag={this._handleEditorsDividerDrag}
Expand Down
2 changes: 1 addition & 1 deletion src/containers/Console.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ function mapStateToProps(state) {
currentCompiledProjectKey: getCurrentCompiledProjectKey(state),
currentProjectKey: getCurrentProjectKey(state),
history: getConsoleHistory(state),
isOpen: !getHiddenUIComponents(state).includes('console'),
isOpen: !getHiddenUIComponents(state).console,
isTextSizeLarge: isTextSizeLarge(state),
requestedFocusedLine: getRequestedFocusedLine(state),
isHidden: !isCurrentProjectSyntacticallyValid(state),
Expand Down
2 changes: 1 addition & 1 deletion src/containers/ErrorReport.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ function mapStateToProps(state) {
function mapDispatchToProps(dispatch) {
return {
onErrorClick(language, line, column) {
dispatch(focusLine(`editor.${language}`, line, column));
dispatch(focusLine(language, line, column));
},
};
}
Expand Down
2 changes: 1 addition & 1 deletion src/containers/Instructions.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ function mapStateToProps(state) {
return {
instructions: getCurrentProjectInstructions(state),
isEditing: isEditingInstructions(state),
isOpen: !getHiddenUIComponents(state).includes('instructions'),
isOpen: !getHiddenUIComponents(state).instructions,
projectKey: getCurrentProjectKey(state),
};
}
Expand Down
8 changes: 8 additions & 0 deletions src/records/HiddenUIComponent.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import {Record} from 'immutable';

export default Record({
componentType: '',
Copy link
Contributor

Choose a reason for hiding this comment

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

I would probably just make this default to null too?

language: null,
line: null,
column: null,
}, 'HiddenUIComponent');
29 changes: 25 additions & 4 deletions src/records/Project.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,25 @@
import {Record, Set} from 'immutable';
import {Record, Set, Map} from 'immutable';
import isArray from 'lodash/isArray';
import isPlainObject from 'lodash/isPlainObject';

import HTML_TEMPLATE from '../../templates/new.html';
import HiddenUIComponent from './HiddenUIComponent';

function hiddenUIArrayToObject(hiddenUIComponents) {
let obj = {};
if (isPlainObject(hiddenUIComponents)) {
obj = hiddenUIComponents;
} else if (isArray(hiddenUIComponents)) {
let component;
for (component of hiddenUIComponents) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about for (const component of hiddenUIComponents) ?

const [componentType, language] = component.split('.');
const componentKey = language || componentType;
obj[componentKey] = new HiddenUIComponent({componentType, language});
}
}
obj.console = new HiddenUIComponent({componentType: 'console'});
return obj;
}

const Sources = Record({
html: HTML_TEMPLATE,
Expand All @@ -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'}),
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

}),
updatedAt: null,
instructions: '',
}) {
static fromJS({
projectKey = null,
sources = {},
enabledLibraries = [],
hiddenUIComponents = [],
hiddenUIComponents = {},
updatedAt = null,
instructions = '',
}) {
return new Project({
projectKey,
sources: new Sources(sources),
enabledLibraries: new Set(enabledLibraries),
hiddenUIComponents: new Set(hiddenUIComponents),
hiddenUIComponents: new Map(hiddenUIArrayToObject(hiddenUIComponents)),
updatedAt,
instructions,
});
Expand Down
2 changes: 2 additions & 0 deletions src/records/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import ConsoleError from './ConsoleError';
import Error from './Error';
import ErrorList from './ErrorList';
import ErrorReport from './ErrorReport';
import HiddenUIComponent from './HiddenUIComponent';
import Project from './Project';

export {
Expand All @@ -13,5 +14,6 @@ export {
Error,
ErrorList,
ErrorReport,
HiddenUIComponent,
Project,
};
Loading