Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for component data-test-* attributes without values #57

Merged

Conversation

Turbo87
Copy link
Collaborator

@Turbo87 Turbo87 commented Jan 23, 2017

This PR is essentially #55 with a small change that restores Ember 1.13 compatibility.

HeroicEric and others added 2 commits January 22, 2017 22:55
This adds support for `{{my-component data-test-foo}}`.

Basically, this adds a handlebars transform that converts the previous
example to `{{my-component data-test-foo=true}}`.
@@ -0,0 +1,42 @@
/* eslint-env node */
var TEST_SELECTOR_PREFIX = /data-test-.*/;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we import this from elsewhere so we don't have to define it at several locations? We also might want to make it configurable in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but I'd like to do that in a separate PR since we already have quite a bit of duplication around that regex

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -9,3 +9,5 @@
<div class="test5">{{data-test-component data-test="foo"}}</div>

<div class="test6">{{data-test-component data-non-test="foo"}}</div>

<div class="test7">{{data-test-component data-test-without-value}}</div>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a test for the block form as well:

<div class="test8">{{#data-test-component data-test-without-value}}{{/data-test-component}}</div>

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -9,3 +9,5 @@
<div class="test5">{{data-test-component data-test="foo"}}</div>

<div class="test6">{{data-test-component data-non-test="foo"}}</div>

<div class="test7">{{data-test-component data-test-without-value}}</div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would a test case for {{component data-test}} similar to .test5 we have now be helpful?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -0,0 +1,42 @@
/* eslint-env node */
var TEST_SELECTOR_PREFIX = /data-test-.*/;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have such a regex already in addon/utils/bind-data-test-attributes.js. I think it makes sense to refactor this into a shared function like isTestSelector. Would that make sense?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the problem is that one uses ES6 modules and the other CommonJS, so reducing duplication there might not be quite as easy

@Turbo87 Turbo87 merged commit 817c645 into mainmatter:master Jan 23, 2017
@Turbo87 Turbo87 deleted the add-support-for-component-test-selectors branch January 23, 2017 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants