Fix the draggable attribute & flesh out enumerated property support. #216
Conversation
The dragabble attribute was not being correctly recognised - when specified it must have only two values, the strings "true" and "false". Previously it was treated as a boolean attribute, which lead to true being accepted while false did not work at all. Change this so the only acceptable values are the two supported strings. Add some belt and braces when specifying the value in an object spec so you cannot accidentally specify an unsupported value.
alexjeffburke
force-pushed
the
feature/supportEnumeratedDraggableAttribute
branch
from
d00e763
to
43fc8d9
Compare
|
@sunesimonsen this actually affects unexpected-reaction - I had a test here that failed because we used the HTML5 DnD backend, something ended up with a draggable attribute and it didn't work. I've looked into the HTML5 spec and the draggable property cannot be treated as a boolean - despite using true/false the value must be fully specified as for example In order to make sure we're still being helpful, should you accidentally specify |
lib/index.js
Outdated
| .appendInspected( | ||
| enumeratedAttributeValues[attributeName] | ||
| ); | ||
| } |
There was a problem hiding this comment.
Is it really necessary to state the supported values?
There was a problem hiding this comment.
If we are going to do this, we shoudn't use withError inside bubbleError as bubbleError is just sugar for withError. We should just set the error mode on the error directly.
There was a problem hiding this comment.
Sorry - I see this is just another sanity check. Then we should just change the bubbleError code.
|
Very nice find and great with a PR - thanks. |
|
@sunesimonsen thanks for picking it up! I wasn't sure I got rhebputput portion totally right but wanted to get a fix together. Hope the supported values thing made sense, just felt like it might be a nasty footgun otherwise. Having a quick look at your additions now :) |
|
Particularly like the small output tweak to remove the array syntax - no room for confusion now 👍 |
sunesimonsen
deleted the
feature/supportEnumeratedDraggableAttribute
branch
|
Released as unexpected-dom@4.6.3 |
The dragabble attribute was not being correctly recognised - when
specified it must have only two values, the strings "true" and
"false". Previously it was treated as a boolean attribute, which
lead to true being accepted while false did not work at all.
Change this so the only acceptable values are the two supported
strings. Add some belt and braces when specifying the value in an
object spec so you cannot accidentally specify an unsupported value.