Widget: Avoid adding extra spaces to the result of this._classes(...) #1372
Conversation
|
Why does this matter? Once the value is passed to |
|
Well, my Chrome Dev Tools show a superfluous space without this change, and no superfluous space with this change. |
|
Good point though. I'll trace further into the code to see why the space survives |
|
OK, so when you do (edited) |
It does work. |
|
@gabrielschulhof your missing a |
|
I'm not sure what you're trying to show. Nobody is saying there isn't a space, but that is extremely different than it not working. |
|
Sorry! I was being ambiguous. By "working", I meant it should produce nice, clean output, just like core's class manipulation methods. I guess we can be lax about it but it looks extremely unprofessional when used in combination with HTML snippets. If core can take the trouble to make sure that class names in the class attribute are separated by exactly one space, with no leading or trailing whitespace, we should also take the same trouble. |
| // The empty string is a valid value for a class key, but pushing it into the array | ||
| // which we then join into a string below will result in our return value having | ||
| // superfluous spaces. Let's only add the value of the key if it's a non-empty string. | ||
| if ( typeof classes[ parts[ i ] ] === "string" && classes[ parts[ i ] ] !== "" ) { |
There was a problem hiding this comment.
Why can't this just be if ( classes[ parts[ i ] ] )?
There was a problem hiding this comment.
I was actually hoping we might discuss this. I had exactly that simple check there at first, but then I thought to myself: do we want to be lax about checking the type here, or do we want to avoid ending up with "ns-structure-class [object Object]" or whatever, or things like "ns-structure-class true".
I mean, these are option values, so they come straight from the developer at worst (if the instance is created with options), or from the framework developer at best. Either way, we may want to consider them "outside, untrusted" or whatever.
Nevertheless, if we consider the simple check you mention to be sufficient, I'm certainly happy to update the PR.
There was a problem hiding this comment.
Let's use the simple check and a short comment like this:
// Avoid adding extra spaces from empty parts
if ( classes[ parts[ i ] ] ) {08c5b7d to
5404f7f
Compare
| "test2": "class3" | ||
| } | ||
| }, | ||
| _create: function() { | ||
| equal( this._classes( "test" ), "test class1 class2" ); | ||
| equal( this._classes( "test2" ), "test2 class3" ); | ||
| deepEqual( this._classes( "testEmpty" ), "testEmpty", |
There was a problem hiding this comment.
Use equal() to match the other assertions.
78b0e5e to
4613cec
Compare
|
Looks good to me. |
4613cec to
ba30279
Compare
|
@arschmitz has merged this locally; it will close when he pushes the rest of his updates for the classes branch. |
|
These changes are now part of PR #1369 so i'm going to close this. |
This is a PR on top of a PR (#1369).