Show whitespace character for default values #203
Conversation
| /** | ||
| * Remove quotes around given string. | ||
| * | ||
| * @param {string} string | ||
| * @returns {string} | ||
| */ | ||
| export function unquote(string) { | ||
| return trim(string, '"\''); | ||
| return string.replace(/^\'|^\"|\"$|\'$/g, ''); |
There was a problem hiding this comment.
Good catch ;-) But I’d even replace it with this module, it also unescape quotes inside the sting: https://github.com/dominictarr/quote-unquote/blob/master/index.js#L14-L20
There was a problem hiding this comment.
Hm, we only "unquote" so that we don't show the quotes surrounding the stringified values parsed from the defaultProp definitions right? E.g. I wouldn't necessarily want the quotes within my defaultProps touched at all.
Also, the function itself looks broken:
var unquote = function (s) {
var quote = s[0]
var single = quote === "'"
return s.substring(1, s.length - 1)
.replace(/\\\\/g, '\\')
.replace(single ? /\\'/g : /\\"/g, quote)
}
unquote("here's my quote"); // "ere's my quot"It seems to eat the first and last char.
EDIT: Oh, I see, it's expecting the string as:
unquote("\"here's my quote\"");I still think the simple regex that's there is sufficient 😛
There was a problem hiding this comment.
It assumes (for some unknown) reason that the string is quoted 8-)
I was thinking that unescaping will make string easier to read but probably we don’t have quotes inside strings to often to do anything.
| * @returns {string} | ||
| */ | ||
| export function showSpaces(string) { | ||
| return string.replace(/^\s|\s$/g, '␣'); |
There was a problem hiding this comment.
Maybe it’d be more useful to show different kinds of whitespace: space, new lines, etc.
There was a problem hiding this comment.
I think it makes sense only to show those if they are at the beginning or end of a line, so I can add support for replacing \n with ⏎
For example, I wouldn't want to see this:
text
here
Represented as:
text⏎here
Also, I imagine a string that starts/ends with a new-line would be an extremely rare case, but I can still add it if you think it makes sense. Let me know!
There was a problem hiding this comment.
I think you’re right, so leave it as is ;-)
|
|
Closes #196