Implement supportsDataTestProperties to disable tagless component assertion #291
Conversation
There was a problem hiding this comment.
nice work, @bendemboski! pretty much exactly what I had in mind 😅
| }); | ||
|
|
||
| bindDataTestAttributes(instance); | ||
| assert.ok(true, 'did not throw'); |
There was a problem hiding this comment.
I would prefer assert.expect(0); at the top of the test instead of this constant-condition assertion
I wish QUnit had a doesNotThrow() assertion...
| let message = `ember-test-selectors could not bind data-test-* properties on ${component} ` + | ||
| `automatically because tagName is empty.`; | ||
| let message = `ember-test-selectors could not bind data-test-* properties on ${component} ` + | ||
| `automatically because tagName is empty.`; |
There was a problem hiding this comment.
might make sense to add a link here to the new README section. something like "if this was intentional have a look at xyz to see how to avoid this assertion"
|
Thanks for your work on this @bendemboski , excited to get this in for AddonDocs & our own projects! |
|
Thanks @bendemboski waiting to get this released! |
|
@samselikoff @ssutar The update on this PR is that I'm waiting on @Turbo87 to reply to this comment to ensure that we want to add code to disable the assertion, rather than just removing it entirely. |
bendemboski
force-pushed
the
allow-tagless
branch
from
160814c to
7382299
Compare
|
@Turbo87 pushed a commit with PR feedback, but didn't squash to make review easier -- feel free to squash/rebase when you merge as I don't think the second commit really needs to be its own commit for posterity. Also, in case your notifications came out of order or something, please have a look at this comment before merging! |
|
@bendemboski @Turbo87 Any update on this would be released? |
|
@bendemboski @Turbo87 Thanks! I tried using this PR locally however it throws an error: I tried to fix it with #323 which essentially extends this PR, please have a look. This is a blocker for native classes migration as it has babel 7 support as a prerequisite. I am currently working on ES6 classes codemods with @pzuraq Your feedback is much appreciated!! |
|
thanks for testing and coming up with a fix @ssutar, I'll review in details once more as soon as I find a bit of free time |
|
closing in favor of #323 |
Fixes #290