Make line numbers in the editor optional #879

xxdavid · 2017-09-20T20:47:08Z

Add an option to disable the line numbers in the Markdown editor.

asmsuechan · 2017-10-01T10:58:56Z

asmsuechan

Please add it in snippet note

asmsuechan · 2017-10-01T11:06:11Z

browser/components/CodeEditor.js

@@ -50,7 +50,7 @@ export default class CodeEditor extends React.Component {
    this.value = this.props.value
    this.editor = CodeMirror(this.refs.root, {
      value: this.props.value,
-      lineNumbers: true,
+      lineNumbers: this.props.lineNumber,


I prefer lineNumbers as the variable name.

Yeah, I prefer lineNumbers too. I used lineNumber as it was already used somewhere else in the code.

it was already used somewhere else in the code.

Where is it? I could not find it...
Does it mean you chose lineNumber because there are probabilities to conflict?

It's used for the line numbers in the preview. Here, for example.

I see. https://github.com/BoostIO/Boostnote/blob/95d74d1ca2206ed18add0348682af0063a5ac5b0/browser/finder/NoteDetail.js#L196

So should I change it to lineNumbers?

asmsuechan · 2017-10-01T11:08:28Z

browser/components/MarkdownEditor.js

@@ -213,6 +213,8 @@ class MarkdownEditor extends React.Component {
    if (!(editorFontSize > 0 && editorFontSize < 101)) editorFontSize = 14
    let editorIndentSize = parseInt(config.editor.indentSize, 10)
    if (!(editorFontSize > 0 && editorFontSize < 132)) editorIndentSize = 4
+    let editorLineNumber = config.editor.lineNumber


I don't think the name editorLineNumber is appropriate because it's a boolean.

Is editorShowLineNumbers better? Or just showLineNumbers? Or what do you propose?

isLineNumberShown? Umm, I'm not sure if it's better...

I don't think that fits the plural form (lineNumbers) you suggested...

I often use is even if the var name is plural, and the prefix is describes it's a boolean value. isLineNumbersShown...?
Did you mean this kind of thing?

https://stackoverflow.com/questions/12960554/java-boolean-getters-is-vs-are

OK then. I meant just the language (English) thing. Other than that, I think isLineNumbersShown is fine.

asmsuechan · 2017-10-01T11:09:14Z

browser/components/MarkdownEditor.js

@@ -213,6 +213,8 @@ class MarkdownEditor extends React.Component {
    if (!(editorFontSize > 0 && editorFontSize < 101)) editorFontSize = 14
    let editorIndentSize = parseInt(config.editor.indentSize, 10)
    if (!(editorFontSize > 0 && editorFontSize < 132)) editorIndentSize = 4
+    let editorLineNumber = config.editor.lineNumber
+    if (editorLineNumber === undefined) editorLineNumber = true


const editorLineNumber = config.editor.lineNumber || true or something?

If config.editor.lineNumber were false (disabled), config.editor.lineNumber || true would be true. I don't think that's what you want.

What are cases that config.editor.lineNumber to be undefined? The setting should be true or false.

Oh, sorry, of course, you are right. I used that before I declared the property in the ConfigManager.js.

If config.editor.lineNumber were false (disabled), config.editor.lineNumber || true would be true. I don't think that's what you want.

I got it.

~~const editorLineNumber = config.editor.lineNumber === undefined ? config.editor.lineNumber : true?~~

I'm confused, sorry. The variable can't be undefined (as you said), can it?

Oh, I was confused, too lol

Forget about the ternary operation.

Or we can just use config.editor.lineNumber directly here, I think 😄 .

asmsuechan · 2017-10-01T11:19:02Z

browser/main/Detail/SnippetNoteDetail.js

@@ -530,6 +530,7 @@ class SnippetNoteDetail extends React.Component {
            fontSize={editorFontSize}
            indentType={config.editor.indentType}
            indentSize={editorIndentSize}
+            lineNumber


I thought Snippet notes should always have line numbers. Shouldn't they?
(lineNumber is equal to lineNumber={true}.)

I think a little regarding the specification.

(lineNumber is equal to lineNumber={true}.)

Understood.

So do you want Snippet notes to respect this setting or should they always show the line numbers?

kazup01 · 2017-10-26T04:57:23Z

Hi @asmsuechan and @xxdavid , how is this state?

xxdavid · 2017-10-26T07:33:02Z

@kazup01, I'm waiting for @asmsuechan to tell what we have agreed on and what to change.

sota1235 · 2017-11-05T06:31:52Z

browser/components/CodeEditor.js

@@ -141,6 +141,10 @@ export default class CodeEditor extends React.Component {
      this.editor.setOption('indentWithTabs', this.props.indentType !== 'space')
    }

+    if (prevProps.lineNumber !== this.props.lineNumber) {
+      this.editor.setOption('lineNumbers', this.props.lineNumber)
+    }


I think you can set even if prevProps.lineNumber !== this.props.lineNumber is false.
Is it for performance?

Yes, it is and as you can see, the checks are performed for all the options in the componentDidUpdate method.

sota1235 · 2017-11-05T06:33:28Z

@xxdavid Hi! I'm one of maintainers of Boostnote.
How's this PR status? I think you got some advices from @asmsuechan .
After you fix, plz tell me. Then I will check and merge!
Thank you

kazup01 · 2017-11-20T07:17:10Z

Hi @asmsuechan , we are waiting for your reply. Could you check @xxdavid's comments ?

xxdavid · 2017-11-20T12:22:18Z

Sorry for still not updating the PR, but I'm unsure about the variable naming (lineNumbers/lineNumbers) and whether should snippet notes respect this option (and don't show the line numbers if it is disabled) or show it always.

sferra · 2017-12-21T14:34:12Z

@xxdavid just in case you are still pondering on the variable naming: maybe consider the name displayLineNumbers.

i would also suggest handling the snippets separately (as in: new ticket/discussion, new pull request). that way this pull request could be closed while still keeping the discussion/issue alive.

xxdavid · 2017-12-23T21:30:08Z

@sferra, yeah, I agree, this suits the purpose best in my opinion.
I'll change all the variables to displayLineNumbers.

…s too

xxdavid · 2017-12-23T21:58:49Z

I've pushed the changes. I also changed snippet notes to respect the option.
Could it be merged now, please?
And merry Christmas to you all! :)

Rokt33r · 2018-01-25T18:58:55Z

@xxdavid
Sorry for being late. I'll review this on this weekend and release within the next week.

Make line numbers in the editor optional

7a4258b

asmsuechan reviewed Oct 1, 2017

View reviewed changes

sota1235 reviewed Nov 5, 2017

View reviewed changes

kazup01 added awaiting review ❇️ Pull request is awaiting a review. awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. and removed awaiting review ❇️ Pull request is awaiting a review. labels Nov 7, 2017

kazup01 assigned sota1235 Dec 9, 2017

kazup01 added awaiting review ❇️ Pull request is awaiting a review. and removed awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels Dec 9, 2017

sota1235 removed their assignment Dec 13, 2017

kazup01 added awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. and removed awaiting review ❇️ Pull request is awaiting a review. labels Dec 14, 2017

xxdavid added 2 commits December 23, 2017 22:51

Rename 'lineNumber' to 'displayLineNumbers'

1f5f6c3

Make the option to disable line numbers in editor affect snippet note…

8506535

…s too

Merge branch 'master' into feature-editor-line-lines

da1098e

Rokt33r approved these changes Feb 3, 2018

View reviewed changes

Rokt33r merged commit a64f73c into BoostIO:master Feb 3, 2018

Rokt33r added next release (v0.8.21) and removed awaiting changes 🖊️ Pull request has been reviewed, but contributor needs to make changes. labels Feb 3, 2018

kazup01 removed the next release (v0.8.21) label Feb 3, 2018

Make line numbers in the editor optional #879

Make line numbers in the editor optional #879

Conversation

xxdavid commented Sep 20, 2017

asmsuechan commented Oct 1, 2017

asmsuechan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asmsuechan Oct 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asmsuechan Oct 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xxdavid Oct 1, 2017 • edited Loading

Choose a reason for hiding this comment

asmsuechan Oct 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asmsuechan Oct 1, 2017 • 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

asmsuechan Oct 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asmsuechan Oct 1, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kazup01 commented Oct 26, 2017

xxdavid commented Oct 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sota1235 commented Nov 5, 2017

kazup01 commented Nov 20, 2017

xxdavid commented Nov 20, 2017

sferra commented Dec 21, 2017

xxdavid commented Dec 23, 2017

xxdavid commented Dec 23, 2017

Rokt33r commented Jan 25, 2018

asmsuechan Oct 1, 2017 •

edited

Loading

asmsuechan Oct 1, 2017 •

edited

Loading

xxdavid Oct 1, 2017 •

edited

Loading

asmsuechan Oct 1, 2017 •

edited

Loading

asmsuechan Oct 1, 2017 •

edited

Loading

asmsuechan Oct 1, 2017 •

edited

Loading

asmsuechan Oct 1, 2017 •

edited

Loading