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.

Sign up for GitHub

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

Add a WYSIWYG-esque editor for instructions #1426

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Add a WYSIWYG-esque editor for instructions #1426

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3073cc9
add react-simplemde-editor dependency
meg2208 Apr 22, 2018
c1ad7d8
WYSIWYG-esque editor for instructions; controlled component pattern
meg2208 Apr 22, 2018
2864084
minor styling for instructions editor
meg2208 Apr 22, 2018
1544c8e
proper behavior on instructions cancel, load new project
meg2208 Apr 22, 2018
c81a8cf
add newline; remove unnecessary id
meg2208 Apr 22, 2018
4bfae0f
move css to simple-mde.css; updates from code review
meg2208 Apr 24, 2018
e2588eb
add logic explanation to getCurrentProjectInstructions
meg2208 Apr 24, 2018
04a6912
displayedInstructions --> draftInstructions
meg2208 Apr 24, 2018
903ad12
fix lint error
meg2208 Apr 24, 2018
d911f6c
disable stylelint for venor css
meg2208 Apr 24, 2018
229e3e1
Merge branch 'master' into instructions-WYSIWYG
meg2208 Apr 24, 2018
19a5f33
add react-coroutine, async load InstructionsEditor and fade in
meg2208 Apr 25, 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
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@
"react-onclickoutside": "^6.5.0",
"react-prevent-clickthrough": "^0.0.3",
"react-redux": "^5.0.3",
"react-simplemde-editor": "^3.6.14",
"reduce-reducers": "^0.1.2",
"redux": "^3.6.0",
"redux-actions": "^2.2.1",
Expand Down
2 changes: 2 additions & 0 deletions src/actions/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
toggleTopBarMenu,
closeTopBarMenu,
startEditingInstructions,
continueEditingInstructions,
cancelEditingInstructions,
} from './ui';

Expand Down Expand Up @@ -97,6 +98,7 @@ export {
toggleTopBarMenu,
closeTopBarMenu,
startEditingInstructions,
continueEditingInstructions,
cancelEditingInstructions,
logIn,
logOut,
Expand Down
6 changes: 6 additions & 0 deletions src/actions/ui.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,12 @@ export const startEditingInstructions = createAction(
(_projectKey, timestamp = Date.now()) => ({timestamp}),
);

export const continueEditingInstructions = createAction(
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

'CONTINUE_EDITING_INSTRUCTIONS',
(projectKey, content) => ({projectKey, content}),
(_projectKey, timestamp = Date.now()) => ({timestamp}),
Copy link
Contributor

Choose a reason for hiding this comment

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

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

);

export const cancelEditingInstructions = createAction(
'CANCEL_EDITING_INSTRUCTIONS',
);
3 changes: 3 additions & 0 deletions src/components/Instructions.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ export default function Instructions({
isOpen,
projectKey,
onCancelEditing,
onContinueEditing,
onSaveChanges,
}) {
if (!isEditing && !instructions || !isOpen) {
Expand All @@ -25,6 +26,7 @@ export default function Instructions({
instructions={instructions}
projectKey={projectKey}
onCancelEditing={onCancelEditing}
onContinueEditing={onContinueEditing}
onSaveChanges={onSaveChanges}
/> :
<div className="instructions">
Expand All @@ -41,5 +43,6 @@ Instructions.propTypes = {
isOpen: PropTypes.bool.isRequired,
projectKey: PropTypes.string.isRequired,
onCancelEditing: PropTypes.func.isRequired,
onContinueEditing: PropTypes.func.isRequired,
onSaveChanges: PropTypes.func.isRequired,
};
31 changes: 15 additions & 16 deletions src/components/InstructionsEditor.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,29 +3,25 @@ import PropTypes from 'prop-types';
import {t} from 'i18next';
import bindAll from 'lodash/bindAll';

import SimpleMDE from 'react-simplemde-editor';

export default class InstructionsEditor extends React.Component {
constructor() {
super();
bindAll(this, '_handleCancelEditing', '_handleSaveChanges', '_ref');
}

componentDidMount() {
if (!this.props.instructions) {
this._editor.focus();
}
bindAll(this, '_handleCancelEditing', '_handleContinueEditing',
'_handleSaveChanges');
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

}

_handleCancelEditing() {
this.props.onCancelEditing();
}

_handleSaveChanges() {
const newValue = this._editor.value.trim();
this.props.onSaveChanges(this.props.projectKey, newValue);
this.props.onSaveChanges(this.props.projectKey, this.props.instructions);
}

_ref(editorElement) {
this._editor = editorElement;
_handleContinueEditing(newValue) {
this.props.onContinueEditing(this.props.projectKey, newValue);
}

render() {
Expand All @@ -46,11 +42,13 @@ export default class InstructionsEditor extends React.Component {
</button>
</div>
<div className="instructions-editor__input-container">
<textarea
className="instructions-editor__input"
defaultValue={this.props.instructions}
placeholder="Type here..."
ref={this._ref}
<SimpleMDE
options={{
autofocus: true,
spellChecker: false,
}}
value={this.props.instructions}
onChange={this._handleContinueEditing}
/>
</div>
<div className="instructions-editor__footer">
Expand All @@ -72,5 +70,6 @@ InstructionsEditor.propTypes = {
instructions: PropTypes.string.isRequired,
projectKey: PropTypes.string.isRequired,
onCancelEditing: PropTypes.func.isRequired,
onContinueEditing: PropTypes.func.isRequired,
onSaveChanges: PropTypes.func.isRequired,
};
4 changes: 4 additions & 0 deletions src/containers/Instructions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
} from '../selectors';
import {
cancelEditingInstructions,
continueEditingInstructions,
updateProjectInstructions,
} from '../actions';

Expand All @@ -25,6 +26,9 @@ function mapDispatchToProps(dispatch) {
onCancelEditing() {
dispatch(cancelEditingInstructions());
},
onContinueEditing(projectKey, newValue) {
dispatch(continueEditingInstructions(projectKey, newValue));
},
onSaveChanges(projectKey, newValue) {
dispatch(updateProjectInstructions(projectKey, newValue));
},
Expand Down
10 changes: 9 additions & 1 deletion src/css/application.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ body {
.instructions-editor__menu {
padding: 0.5rem;
text-align: right;
background-color: var(--color-light-gray);
background-color: var(--color-low-contrast-gray);
}

.instructions-editor__footer {
Expand Down Expand Up @@ -865,3 +865,11 @@ body {
.u__icon_disabled {
cursor: default;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

/**
* simplemde v1.11.2
* Copyright Next Step Webs, Inc.
* @link https://github.com/NextStepWebs/simplemde-markdown-editor
* @license MIT
*/
/* stylelint-disable-line */ .CodeMirror{color:#000}.CodeMirror-lines{padding:4px 0}.CodeMirror pre{padding:0 4px}.CodeMirror-gutter-filler,.CodeMirror-scrollbar-filler{background-color:#fff}.CodeMirror-gutters{border-right:1px solid #ddd;background-color:#f7f7f7;white-space:nowrap}.CodeMirror-linenumber{padding:0 3px 0 5px;min-width:20px;text-align:right;color:#999;white-space:nowrap}.CodeMirror-guttermarker{color:#000}.CodeMirror-guttermarker-subtle{color:#999}.CodeMirror-cursor{border-left:1px solid #000;border-right:none;width:0}.CodeMirror div.CodeMirror-secondarycursor{border-left:1px solid silver}.cm-fat-cursor .CodeMirror-cursor{width:auto;border:0!important;background:#7e7}.cm-fat-cursor div.CodeMirror-cursors{z-index:1}.cm-animate-fat-cursor{width:auto;border:0;-webkit-animation:blink 1.06s steps(1) infinite;-moz-animation:blink 1.06s steps(1) infinite;animation:blink 1.06s steps(1) infinite;background-color:#7e7}@-moz-keyframes blink{50%{background-color:transparent}}@-webkit-keyframes blink{50%{background-color:transparent}}@keyframes blink{50%{background-color:transparent}}.cm-tab{display:inline-block;text-decoration:inherit}.CodeMirror-ruler{border-left:1px solid #ccc;position:absolute}.cm-s-default .cm-header{color:#00f}.cm-s-default .cm-quote{color:#090}.cm-negative{color:#d44}.cm-positive{color:#292}.cm-header,.cm-strong{font-weight:700}.cm-em{font-style:italic}.cm-link{text-decoration:underline}.cm-strikethrough{text-decoration:line-through}.cm-s-default .cm-keyword{color:#708}.cm-s-default .cm-atom{color:#219}.cm-s-default .cm-number{color:#164}.cm-s-default .cm-def{color:#00f}.cm-s-default .cm-variable-2{color:#05a}.cm-s-default .cm-variable-3{color:#085}.cm-s-default .cm-comment{color:#a50}.cm-s-default .cm-string{color:#a11}.cm-s-default .cm-string-2{color:#f50}.cm-s-default .cm-meta,.cm-s-default .cm-qualifier{color:#555}.cm-s-default .cm-builtin{color:#30a}.cm-s-default .cm-bracket{color:#997}.cm-s-default .cm-tag{color:#170}.cm-s-default .cm-attribute{color:#00c}.cm-s-default .cm-hr{color:#999}.cm-s-default .cm-link{color:#00c}.cm-invalidchar,.cm-s-default .cm-error{color:red}.CodeMirror-composing{border-bottom:2px solid}div.CodeMirror span.CodeMirror-matchingbracket{color:#0f0}div.CodeMirror span.CodeMirror-nonmatchingbracket{color:#f22}.CodeMirror-matchingtag{background:rgba(255,150,0,.3)}.CodeMirror-activeline-background{background:#e8f2ff}.CodeMirror{position:relative;overflow:hidden;background:#fff}.CodeMirror-scroll{overflow:scroll!important;margin-bottom:-30px;margin-right:-30px;padding-bottom:30px;height:100%;outline:0;position:relative}.CodeMirror-sizer{position:relative;border-right:30px solid transparent}.CodeMirror-gutter-filler,.CodeMirror-hscrollbar,.CodeMirror-scrollbar-filler,.CodeMirror-vscrollbar{position:absolute;z-index:6;display:none}.CodeMirror-vscrollbar{right:0;top:0;overflow-x:hidden;overflow-y:scroll}.CodeMirror-hscrollbar{bottom:0;left:0;overflow-y:hidden;overflow-x:scroll}.CodeMirror-scrollbar-filler{right:0;bottom:0}.CodeMirror-gutter-filler{left:0;bottom:0}.CodeMirror-gutters{position:absolute;left:0;top:0;min-height:100%;z-index:3}.CodeMirror-gutter{white-space:normal;height:100%;display:inline-block;vertical-align:top;margin-bottom:-30px}.CodeMirror-gutter-wrapper{position:absolute;z-index:4;background:0 0!important;border:none!important;-webkit-user-select:none;-moz-user-select:none;user-select:none}.CodeMirror-gutter-background{position:absolute;top:0;bottom:0;z-index:4}.CodeMirror-gutter-elt{position:absolute;cursor:default;z-index:4}.CodeMirror-lines{cursor:text;min-height:1px}.CodeMirror pre{-moz-border-radius:0;-webkit-border-radius:0;border-radius:0;border-width:0;background:0 0;font-family:inherit;font-size:inherit;margin:0;white-space:pre;word-wrap:normal;line-height:inherit;color:inherit;z-index:2;position:relative;overflow:visible;-webkit-tap-highlight-color:transparent;-webkit-font-variant-ligatures:none;font-variant-ligatures:none}.CodeMirror-wrap pre{word-wrap:break-word;white-space:pre-wrap;word-break:normal}.CodeMirror-linebackground{position:absolute;left:0;right:0;top:0;bottom:0;z-index:0}.CodeMirror-linewidget{position:relative;z-index:2;overflow:auto}.CodeMirror-code{outline:0}.CodeMirror-gutter,.CodeMirror-gutters,.CodeMirror-linenumber,.CodeMirror-scroll,.CodeMirror-sizer{-moz-box-sizing:content-box;box-sizing:content-box}.CodeMirror-measure{position:absolute;width:100%;height:0;overflow:hidden;visibility:hidden}.CodeMirror-cursor{position:absolute}.CodeMirror-measure pre{position:static}div.CodeMirror-cursors{visibility:hidden;position:relative;z-index:3}.CodeMirror-focused div.CodeMirror-cursors,div.CodeMirror-dragcursors{visibility:visible}.CodeMirror-selected{background:#d9d9d9}.CodeMirror-focused .CodeMirror-selected,.CodeMirror-line::selection,.CodeMirror-line>span::selection,.CodeMirror-line>span>span::selection{background:#d7d4f0}.CodeMirror-crosshair{cursor:crosshair}.CodeMirror-line::-moz-selection,.CodeMirror-line>span::-moz-selection,.CodeMirror-line>span>span::-moz-selection{background:#d7d4f0}.cm-searching{background:#ffa;background:rgba(255,255,0,.4)}.cm-force-border{padding-right:.1px}@media print{.CodeMirror div.CodeMirror-cursors{visibility:hidden}}.cm-tab-wrap-hack:after{content:''}span.CodeMirror-selectedtext{background:0 0}.CodeMirror{height:auto;min-height:560px;border:1px solid #ddd;border-bottom-left-radius:4px;border-bottom-right-radius:4px;padding:10px;font:inherit;z-index:1}.CodeMirror-scroll{min-height:560px}.CodeMirror-fullscreen{background:#fff;position:fixed!important;top:50px;left:0;right:0;bottom:0;height:auto;z-index:9}.CodeMirror-sided{width:50%!important}.editor-toolbar{position:relative;opacity:.6;-webkit-user-select:none;-moz-user-select:none;-ms-user-select:none;-o-user-select:none;user-select:none;padding:0 10px;border-top:1px solid #bbb;border-left:1px solid #bbb;border-right:1px solid #bbb;border-top-left-radius:4px;border-top-right-radius:4px}.editor-toolbar:after,.editor-toolbar:before{display:block;content:' ';height:1px}.editor-toolbar:before{margin-bottom:8px}.editor-toolbar:after{margin-top:8px}.editor-toolbar:hover,.editor-wrapper input.title:focus,.editor-wrapper input.title:hover{opacity:.8}.editor-toolbar.fullscreen{width:100%;height:50px;overflow-x:auto;overflow-y:hidden;white-space:nowrap;padding-top:10px;padding-bottom:10px;box-sizing:border-box;background:#fff;border:0;position:fixed;top:0;left:0;opacity:1;z-index:9}.editor-toolbar.fullscreen::before{width:20px;height:50px;background:-moz-linear-gradient(left,rgba(255,255,255,1) 0,rgba(255,255,255,0) 100%);background:-webkit-gradient(linear,left top,right top,color-stop(0,rgba(255,255,255,1)),color-stop(100%,rgba(255,255,255,0)));background:-webkit-linear-gradient(left,rgba(255,255,255,1) 0,rgba(255,255,255,0) 100%);background:-o-linear-gradient(left,rgba(255,255,255,1) 0,rgba(255,255,255,0) 100%);background:-ms-linear-gradient(left,rgba(255,255,255,1) 0,rgba(255,255,255,0) 100%);background:linear-gradient(to right,rgba(255,255,255,1) 0,rgba(255,255,255,0) 100%);position:fixed;top:0;left:0;margin:0;padding:0}.editor-toolbar.fullscreen::after{width:20px;height:50px;background:-moz-linear-gradient(left,rgba(255,255,255,0) 0,rgba(255,255,255,1) 100%);background:-webkit-gradient(linear,left top,right top,color-stop(0,rgba(255,255,255,0)),color-stop(100%,rgba(255,255,255,1)));background:-webkit-linear-gradient(left,rgba(255,255,255,0) 0,rgba(255,255,255,1) 100%);background:-o-linear-gradient(left,rgba(255,255,255,0) 0,rgba(255,255,255,1) 100%);background:-ms-linear-gradient(left,rgba(255,255,255,0) 0,rgba(255,255,255,1) 100%);background:linear-gradient(to right,rgba(255,255,255,0) 0,rgba(255,255,255,1) 100%);position:fixed;top:0;right:0;margin:0;padding:0}.editor-toolbar a{display:inline-block;text-align:center;text-decoration:none!important;color:#2c3e50!important;width:30px;height:30px;margin:0;border:1px solid transparent;border-radius:3px;cursor:pointer}.editor-toolbar a.active,.editor-toolbar a:hover{background:#fcfcfc;border-color:#95a5a6}.editor-toolbar a:before{line-height:30px}.editor-toolbar i.separator{display:inline-block;width:0;border-left:1px solid #d9d9d9;border-right:1px solid #fff;color:transparent;text-indent:-10px;margin:0 6px}.editor-toolbar a.fa-header-x:after{font-family:Arial,"Helvetica Neue",Helvetica,sans-serif;font-size:65%;vertical-align:text-bottom;position:relative;top:2px}.editor-toolbar a.fa-header-1:after{content:"1"}.editor-toolbar a.fa-header-2:after{content:"2"}.editor-toolbar a.fa-header-3:after{content:"3"}.editor-toolbar a.fa-header-bigger:after{content:"▲"}.editor-toolbar a.fa-header-smaller:after{content:"▼"}.editor-toolbar.disabled-for-preview a:not(.no-disable){pointer-events:none;background:#fff;border-color:transparent;text-shadow:inherit}@media only screen and (max-width:700px){.editor-toolbar a.no-mobile{display:none}}.editor-statusbar{padding:8px 10px;font-size:12px;color:#959694;text-align:right}.editor-statusbar span{display:inline-block;min-width:4em;margin-left:1em}.editor-preview,.editor-preview-side{padding:10px;background:#fafafa;overflow:auto;display:none;box-sizing:border-box}.editor-statusbar .lines:before{content:'lines: '}.editor-statusbar .words:before{content:'words: '}.editor-statusbar .characters:before{content:'characters: '}.editor-preview{position:absolute;width:100%;height:100%;top:0;left:0;z-index:7}.editor-preview-side{position:fixed;bottom:0;width:50%;top:50px;right:0;z-index:9;border:1px solid #ddd}.editor-preview-active,.editor-preview-active-side{display:block}.editor-preview-side>p,.editor-preview>p{margin-top:0}.editor-preview pre,.editor-preview-side pre{background:#eee;margin-bottom:10px}.editor-preview table td,.editor-preview table th,.editor-preview-side table td,.editor-preview-side table th{border:1px solid #ddd;padding:5px}.CodeMirror .CodeMirror-code .cm-tag{color:#63a35c}.CodeMirror .CodeMirror-code .cm-attribute{color:#795da3}.CodeMirror .CodeMirror-code .cm-string{color:#183691}.CodeMirror .CodeMirror-selected{background:#d9d9d9}.CodeMirror .CodeMirror-code .cm-header-1{font-size:200%;line-height:200%}.CodeMirror .CodeMirror-code .cm-header-2{font-size:160%;line-height:160%}.CodeMirror .CodeMirror-code .cm-header-3{font-size:125%;line-height:125%}.CodeMirror .CodeMirror-code .cm-header-4{font-size:110%;line-height:110%}.CodeMirror .CodeMirror-code .cm-comment{background:rgba(0,0,0,.05);border-radius:2px}.CodeMirror .CodeMirror-code .cm-link{color:#7f8c8d}.CodeMirror .CodeMirror-code .cm-url{color:#aab2b3}.CodeMirror .CodeMirror-code .cm-strikethrough{text-decoration:line-through}.CodeMirror .CodeMirror-placeholder{opacity:.5}.CodeMirror .cm-spell-error:not(.cm-url):not(.cm-comment):not(.cm-tag):not(.cm-word){background:rgba(255,0,0,.15)}
10 changes: 10 additions & 0 deletions src/reducers/ui.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export const DEFAULT_WORKSPACE = new Immutable.Map({
columnFlex: DEFAULT_COLUMN_FLEX,
rowFlex: DEFAULT_ROW_FLEX,
isDraggingColumnDivider: false,
displayedInstructions: '',
Copy link
Contributor

Choose a reason for hiding this comment

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

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

isEditingInstructions: false,
});

Expand Down Expand Up @@ -224,7 +225,16 @@ export default function ui(stateIn, action) {
case 'START_EDITING_INSTRUCTIONS':
return state.setIn(['workspace', 'isEditingInstructions'], true);

case 'CONTINUE_EDITING_INSTRUCTIONS':
return state.setIn(
['workspace', 'displayedInstructions'],
action.payload.content,
);

case 'CANCEL_EDITING_INSTRUCTIONS':
return state.setIn(['workspace', 'isEditingInstructions'], false).
setIn(['workspace', 'displayedInstructions'], '');

case 'UPDATE_PROJECT_INSTRUCTIONS':
return state.setIn(['workspace', 'isEditingInstructions'], false);

Expand Down
21 changes: 18 additions & 3 deletions src/selectors/getCurrentProjectInstructions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,24 @@
import {createSelector} from 'reselect';
import getCurrentProjectKey from './getCurrentProjectKey';
import getCurrentProjectInstructionsUnsaved
from './getCurrentProjectInstructionsUnsaved';
import isEditingInstructions from './isEditingInstructions';
import getProjects from './getProjects';

export default createSelector(
[getCurrentProjectKey, getProjects],
(projectKey, projects) =>
projectKey ? projects.getIn([projectKey, 'instructions']) : '',
[
getCurrentProjectKey, getProjects,
isEditingInstructions, getCurrentProjectInstructionsUnsaved,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

],
(projectKey, projects, isEditing, instructionUnsaved) => {
if (!projectKey) {
return '';
}

if (isEditing && instructionUnsaved) {
return instructionUnsaved;
Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 👍

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 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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

}

return projectKey ? projects.getIn([projectKey, 'instructions']) : '';
},
);
3 changes: 3 additions & 0 deletions src/selectors/getCurrentProjectInstructionsUnsaved.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function getCurrentProjectinstructionsUnsaved(state) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

return state.getIn(['ui', 'workspace', 'displayedInstructions']);
}
10 changes: 9 additions & 1 deletion test/unit/reducers/ui.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ test('startEditingInstructions', reducerTest(
initialState.setIn(['workspace', 'isEditingInstructions'], true),
));

test('startEditingInstructions', reducerTest(
test('cancelEditingInstructions', reducerTest(
reducer,
initialState.setIn(['workspace', 'isEditingInstructions'], true),
cancelEditingInstructions,
Expand All @@ -148,6 +148,14 @@ test('updateProjectInstructions', reducerTest(
initialState,
));

test('cancelEditingInstructions after updating', reducerTest(
reducer,
initialState.setIn(['workspace', 'isEditingInstructions'], true).
setIn(['workspace', 'displayedInstructions'], 'foo'),
cancelEditingInstructions,
initialState,
));

test('gistNotFound', reducerTest(
reducer,
initialState,
Expand Down
Loading