Ignore nonsignificant whitespace in selectors #348
Conversation
| @@ -29,7 +29,7 @@ export function childrenOfNode(node) { | |||
|
|
|||
| export function hasClassName(node, className) { | |||
| const classes = propsOfNode(node).className || ''; | |||
| return ` ${classes} `.indexOf(` ${className} `) > -1; | |||
| return ` ${classes.trim()} `.indexOf(` ${className} `) > -1; | |||
There was a problem hiding this comment.
trim won't catch foo\nbar, will it?
I think you may instead want to .replace(/\s*/g, ' ')
There was a problem hiding this comment.
I guess I'm unsure of what the expected behavior is supposed to be in that case. This change was meant to just address non-significiant (leading/trailing) whitespace. If we're coercing all whitespace characters to spaces I think that's a bit different. A user might have some (strange) reason to have a new-line there instead of a space?
Also:
'foo\nbar'.replace(/\s*/g, ' ')
// " f o o b a r "Even if we just do .replace(/\s/g, ' ') cases like foo\nbar\n would return a string ("foo bar ") that still needs to be trimmed.
There was a problem hiding this comment.
Also, DOM API's like document.querySelector will ignore leading/trailing whitespace but not significant whitespace, which is kind of what I was following here. If you have significant whitespace in your class names (which you probably shouldn't anyways) you should test explicitly for that.
What do you think?
There was a problem hiding this comment.
It's a class name - the browser will always collapse whitespace into a single space anyways, I thought.
I totally agree though that what document.querySelector does should be our guide here - but i was under the impression that the array of class names on an element was as if you'd done .split(/\s+/g) on it.
There was a problem hiding this comment.
At least in Chrome the new line characters are perserved (if I check node.className on an element with the class foo\nbar it prints with the new line) but I'm not entirely sure how consistent that is.
But you're right, checking classList does still parse it as two classes as it should. For some reason I thought we were talking about parsing foo\nbar as a singular class name. I'll add update the PR to handle that case as well.
|
@ljharb updated the PR to handle significant whitespace as well 👍 |
aweary
force-pushed
the
selector-whitespace
branch
2 times, most recently
from
7838753
to
5adbced
Compare
| @@ -51,7 +51,8 @@ export function instHasClassName(inst, className) { | |||
| if (!isDOMComponent(inst)) { | |||
| return false; | |||
| } | |||
| const classes = findDOMNode(inst).className || ''; | |||
| let classes = findDOMNode(inst).className || ''; | |||
| classes = classes.replace(/\s/g, ' ').trim(); | |||
There was a problem hiding this comment.
shouldn't the trim now be unnecessary?
There was a problem hiding this comment.
You're right, removed 👍
aweary
force-pushed
the
selector-whitespace
branch
from
5adbced
to
c64e776
Compare
|
LGTM |
|
@aweary why did you close this? |
|
I...didn't, I have no idea why that happened. |
|
👍 |
Resolves #347