Changed "Enter Value" to "Add Remarks"

[KeeyanGhoreshi]

Fixes #219 with a CSS change, making the content when the text area is empty "Add Remarks" instead of "Enter Value", and restricting the behavior to the first child of the parent span so that empty lines don't have "Add Remarks" on them.

Previously empty newlines would be considered as an empty span and have their content set to "Enter Value". The new class allows for empty lines, but still will put a stray "Add Remarks" if the first line if it is left empty.

[dehall]

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.

[KeeyanGhoreshi]

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

[KeeyanGhoreshi]

The most recent changes address #68. Instead of a CSS change I edited the behavior of the text area to only allow non-empty spans, since white space gets represented as a <br /> anyway. If there are no spans in the remarks section, an empty one is added to ensure that the "Add Remarks" text shows up. This is because if you add a couple of empty new lines, you'll get a bunch of breaks but no span, and the CSS won't have an empty span to put the "Add Remarks" text in.

These changes allow for free use of white space. There is a very small issue, though, in that the padding changes in the style only affects the first line.

edit: The small padding issue was easier to fix than I thought so everything should be good now.

[arscan]

I put in some comments here. But after taking a look at it running, perhaps an easier thing to do would be to get rid of the spans and use divs, or set the style of the spans to display:block? That way you could remove the <br/>s as the line breaks will come for free, and you could get rid of a lot of this code.

[arscan]

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.

[KeeyanGhoreshi]

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

[arscan]

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

[arscan]

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.

[arscan]

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.

[arscan]

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

[arscan]

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

[arscan]

I'm not sure if this fixes #68 (and my suggestion wouldn't either). I think the intent of that issue is to make it so that remarks that were entered in json, formatted into arrays so that it reads nicely, don't get breaks added when viewing it in the module builder because it causes strange line wrap behavior. It is actually a nontrivial problem to solve, because there are times when we would want to preserve the line breaks... like when somebody puts in a line of ------------------, or maybe if somebody puts in multiple breaks.

[arscan]

Spoke with @KeeyanGhoreshi . He's fixing some of the style suggestions I made, and we'll merge this in after. This isn't attempting to fix #68 after all, we'll tackle that at some point in the future.

[KeeyanGhoreshi]

Speaking of #68, the way remarks are put into the JSON is a bit bizarre. There's no difference between a new line for styling purposes and a new line that's actually desired or useful. New line here means another entry in the array. For a continuous paragraph, its separated out in the JSON and that gets represented in the module builder. Is there a reason to separate it into the array? Shouldn't paragraphs be one entry in the array, instead of multiple?

If you type into the remarks editor, you get one long entry into the array that wraps nicely and doesn't have any issues. For example, modules that were made entirely in the module builder (like my cystic fibrosis module) don't have this issue, paragraphs are one entry in the array.

I don't think this issue really has a "fix", there's no way to concatenate the array into paragraphs because you don't know the intent of the writer. The problem is the styling of the JSON remarks, in that they're hand-wrapped by making multiple entries into the array. But since the remarks editor can be resized, that's going to cause wrapping issues.

[arscan]

Modules used to be be exclusively edited by hand, so the format is optimized for text-editors. You can't represent line breaks in json like this:

   {
      remarks: 'You aren't allowed to do something like this in json
                where multiple lines are put into one single string
                which is why each line is an entry in an array'
   }

instead it would be a long line, which isn't readable at all in traditional editors. It is like having a single line of code be extremely long... sure, it works, but it isn't very readable. So the only real option is to use array elements to represent each line.

I think it is a good practice to have these modules be optimized for human-readability without the requirement to edit them through a tool (e.g. the module builder), as we are using git to review diffs / changes. Anyone that has tried to use git with files that are nominally human readable but really intended to be edited with tool support knows how painful that can get.

Yeah, it isn't easy. I was envisioning putting a few simple rules that give hints for how they should be rendered. Namely, I was thinking that we could do something like wrap single line breaks, but if the 'author' wants to start a new paragraph, or force a new line start, then they have to enter in a blank "". Then we use some kind of line-wrap algorithm on save so that the JSON that gets saved has short lines.

Happy to discuss further if you strongly disagree though.

[arscan]

To be clear though, let's keep those issues separate. If you are interested and available you are free to tackle that issue separately. (I'll reference these comments in that issue, we should really be having this conversation over in #68)