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

Jump to bottom

Changed "Enter Value" to "Add Remarks" #224

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

Changed "Enter Value" to "Add Remarks" #224

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5408f83
Changed "Enter Value" to "Add Remarks"
kghoreshi Aug 3, 2018
d6466d4
changed text area behavior to avoid empty spans being added with breaks
kghoreshi Aug 6, 2018
851316f
removed value set from state
kghoreshi Aug 6, 2018
5a29ade
fixed small padding issue
kghoreshi Aug 6, 2018
0959def
Made stylistic changes and changed spans to divs
kghoreshi Aug 10, 2018
9f6eec3
removed unnecessary import
kghoreshi Aug 10, 2018
ea8f48b
changed remarks to use a blank entry in array as new lines
kghoreshi Aug 16, 2018
1fda699
revert errant unrelated change
KeeyanGhoreshi Aug 16, 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
22 changes: 20 additions & 2 deletions src/components/editor/State.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,15 @@ textarea {
padding-right: .2em;
}

.remarks-text {
font-style: italic;
text-decoration: underline !important;
cursor: text;
color: #336699 !important;
padding-left: .2em;
padding-right: .2em;
}

.editable-error {
color: #ff3300 !important;
}
Expand All @@ -59,12 +68,20 @@ span.editable-text:empty {
color: #ff3300 !important;
}

span.remarks-text:empty:after {
content: 'Add Remarks';
}

span.remarks-text > :empty:after {
content: 'Add Remarks';
}

span.editable-text:empty:after {
content: 'Enter value';
content: 'Enter Value';
}

span.editable-text > :empty:after {
content: 'Enter value';
content: 'Enter Value';
}

.section {
Expand All @@ -91,3 +108,4 @@ span.editable-text > :empty:after {
.warning {
color: #ff3300;
}

9 changes: 5 additions & 4 deletions src/components/editor/State.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import ConditionalEditor from './Conditional';
import Transition from './Transition';
import { getTemplate } from '../../templates/Templates';
import { BasicTutorial, EditTutorial } from '../../templates/Tutorial';

import TextArea from './TextArea'

import './State.css';

Expand Down Expand Up @@ -123,7 +123,6 @@ class StateEditor extends Component<Props> {

let remarks = this.props.state.remarks ||"";
remarks = Array.isArray(remarks)? remarks.join("\n"): remarks;

const transitionType = (this.props.state.transition||{}).type;
return (
<div className="State">
Expand All @@ -133,8 +132,8 @@ class StateEditor extends Component<Props> {
<h3><RIEInput className='editable-text' className='editable-text' propName={'name'} value={this.props.state.name} change={this.props.renameNode} /></h3>
State Type: <RIESelect className='editable-text' className='editable-text' value={{id: this.props.state.type, text: this.props.state.type}} propName='type'change={this.props.changeType} options={typeOptions}/>
<hr/>
<RIETextArea className='editable-text' value={remarks} propName="remarks" change={this.updateRemarks} />
<br/>
<TextArea className='remarks-text' value={remarks} propName="remarks" change={this.updateRemarks} />
<br />
<hr/>
<div className="State-Editor">
{this.renderStateType()}
Expand Down Expand Up @@ -436,6 +435,8 @@ class ConditionOnset extends Component<Props> {
<br />
{this.renderAssignToAttribute()}
<div className='section'>
Value Set: <RIEInput className='value-set-text editable-text' value={state.value_set || ''} propName={'value_set'} change={this.props.onChange('value_set')} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Careful, looks like your value set stuff got added here as well. If you're working on multiple tasks at the same time, it's good to use git add -p <filename> -- the -p flag meaning by parts so it will ask you to confirm each changed section of the file. That way you don't add anything unexpected to the commit.

Copy link
Author

Choose a reason for hiding this comment

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

ah I forgot I had added that. I'll take it out for now its just one line, but will use -p in the future

<br />
Codes
<br />
<Codes codes={state.codes} system={"SNOMED-CT"} onChange={this.props.onChange('codes')} />
Expand Down
49 changes: 49 additions & 0 deletions src/components/editor/TextArea.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import React from 'react';
import ReactDOM from 'react-dom';
import RIEStatefulBase from 'riek/lib/RIEStatefulBase';
export default class TextArea extends RIEStatefulBase {
keyDown = (event) => {
if (event.keyCode === 27) { this.cancelEditing() } // Escape
};

renderEditingComponent = () => {
return <textarea
rows={this.props.rows}
cols={this.props.cols}
disabled={this.state.loading}
className={this.makeClassString()}
defaultValue={this.props.value}
onInput={this.textChanged}
onBlur={this.finishEditing}
ref="input"
onKeyDown={this.keyDown}
{...this.props.editProps} />;
};

renderNormalComponent = () => {
const value = this.state.newValue || this.props.value
const spans_and_brs = []
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: javascript vars don't use this format, rename to spansAndBrs;

var anySpan = false
let i = 0
value.split("\n").map(line => {
Copy link
Member

Choose a reason for hiding this comment

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

This isn't really the right use of a map. If you aren't taking the output of the map, and instead are using it to iterate through the array, use the forEach. I understand why you ended up doing this, as there isn't a 1-1 relationship between input and output elements in the array you are building. If you really wanted to use map, you could have the output here be an array (that may have 1 or 2 elements), and chain flat() to it. I'm personally ok with just using forEach though.

Copy link
Author

@KeeyanGhoreshi KeeyanGhoreshi Aug 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I'm thinking about keeping the map, and using it to return the length of all the lines to see if there are only empty lines:

const lengths = value.split("\n").map(line => { spansAndBrs.push(<div key={spansAndBrs.length}>{line}</div>) return line.length });

I'll probably end up making it a .reduce() though

if(line.length > 0){
anySpan = true
spans_and_brs.push(<span key={i}>{line}</span>)
Copy link
Member

Choose a reason for hiding this comment

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

instead of keeping track of i, perhaps just do spans_and_brs.length

Copy link
Member

Choose a reason for hiding this comment

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

Or, don't do this now, and at the end do something like spans_and_brs.forEach( (el, i) => el.key = i). Or you could do a map, but it seems like forEach is a little less wordy.

}
spans_and_brs.push(<br key={i+1} />)
i += 2
});

spans_and_brs.pop() // remove last br tag
if(!anySpan){
spans_and_brs.length=0
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure you are supposed to ever just set the length of an array to 0 in javascript. What is the purpose of this? To clear it out? If that is the case, just do spans_and_brs = []; Or, in combination with the below line, do spans_and_brs = [<span key="placeholder">{''}</span>];

Maybe I'm misreading the intent of this block though.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, but this would require you to change the spans_and_brs from const to let in the declaration.

spans_and_brs.unshift(<span key="placeholder">{''}</span>)
}
return <span
tabIndex="0"
className={this.makeClassString()}
onFocus={this.startEditing}
onClick={this.startEditing}
{...this.props.defaultProps}>{spans_and_brs}</span>;
};
}