#2308 Go-to only numbers fix #2370
base: master
Are you sure you want to change the base?
Conversation
…sor and center it. Otherwise log an error and show the console
Awesome, thanks for the PR! I'm hesitant to open the console on an error, but should be able to take a look at everything over the weekend. Worst case we leave the console in whatever state the user had it in and log the less verbose error. Edit: I haven't forgotten about this... I've realized how little time I have for LT over the weekend. Plan to get to it this week... ping me if you don't hear back by 5/31. |
Ouuu. I didn't know that there is such thing as "notifos". I find notifos a better approach here than writing it to the console, as I think console should be used for more "important" things. |
src/lt/objs/find.cljs
Outdated
l))}) | ||
(editor/center-cursor cur))))}) | ||
(let [line (if-not (number? l) | ||
(js/parseInt l) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parseInt(string, radix)
It is recommended to add the base for the following reasons:
If radix is undefined or 0 (or absent), JavaScript assumes the following:
- If the input string begins with "0x" or "0X", radix is 16 (hexadecimal) and the remainder of the string is parsed.
- If the input string begins with "0", radix is eight (octal) or 10 (decimal). Exactly which radix is chosen is implementation-dependent. ECMAScript 5 specifies that 10 (decimal) is used, but not all browsers support this yet. For this reason always specify a radix when using parseInt.
- If the input string begins with any other value, the radix is 10 (decimal).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it was not used before, I didn't even think of specifying it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't aware of this either... If you could add that radix parameter of 10 just to be safe, that would be great!
src/lt/objs/find.cljs
Outdated
(let [cur (pool/last-active)] | ||
(editor/move-cursor cur {:ch 0 :line (dec line)}) | ||
(editor/center-cursor cur)) | ||
(notifos/set-msg! (str "Line " l " is not a positive integer > 0") {:class "error"}))))}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clarity, perhaps change the message to be "Line number '" l "' must be a positive integer"
.
Currently, if the input is blank we get "Line is not a positive integer >0" and if the input was "cat" we would get "Line cat is not a positive integer > 0".
They would look like the following with the suggested message:
"Line number '' must be a positive integer"
"Line number 'cat' must be a positive integer"
This looks great @denis631! Notifos seems like a perfect fit here. Just the two minor changes and we should be all set. |
Checking whether or not the line is a positive integer if yes then move cursor and center it. Otherwise log an error and show the console