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 #55

Merged

Conversation

HeroicEric
Copy link
Contributor

@HeroicEric HeroicEric commented Jan 23, 2017

Transforms

{{my-component data-test-foo}}

to:

{{my-component data-test-foo=true}}

Description

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}}, which already works.

I understand if you'd rather not accept this feature but I thought it might be nice since this style selector is supported for regular elements 😄

Use case:

The app I'm currently working on uses a lot of <div data-test-foo> style test selectors. I'd like to extract some of these elements into components but the current situation with the component support would require either coming up with values for each of the data-test-* attributes or using data-test-foo=true throughout the app.

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}}`.
@@ -35,6 +35,14 @@ module.exports = {
plugin: StripTestSelectorsTransform,
baseDir: function() { return __dirname; }
});
} else {
var TransformTestSelectorParamsToHashPairs = require('./transform-test-selector-params-to-hash-pairs');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think? this is where this should be but I'm definitely not sure.

@marcoow marcoow requested a review from Turbo87 January 23, 2017 09:20
@Turbo87
Copy link
Collaborator

Turbo87 commented Jan 23, 2017

@HeroicEric nice work! there is one small caveat though, because you now can no longer pass a data-test-* attribute as a positional argument to a template helper, but I think that's okay since you can still pass it as a named argument. I'm going to review this a little more in depth later today and will hopefully then figure out why some tests are failing here.

@marcoow
Copy link
Member

marcoow commented Jan 23, 2017

What will

{{my-component data-test-foo=true}}

actually render though? I'm thinking if it renders

<div data-test-foo>

that's fine but if it renders

<div data-test-foo="true">

we're adding additional complexity with this.

@Turbo87
Copy link
Collaborator

Turbo87 commented Jan 23, 2017

it should render the first code block, but I'll verify that later.

@Turbo87
Copy link
Collaborator

Turbo87 commented Jan 23, 2017

@marcoow seems to do exactly what we want:

bildschirmfoto 2017-01-23 um 12 48 57

@Turbo87
Copy link
Collaborator

Turbo87 commented Jan 23, 2017

@HeroicEric there were two issues that caused the tests to fail:

  • a typo in one of the tests cause the canary scenario to fail, but that had nothing to do with this PR and got fixed in tests: Fix typo #56

  • you are using the traverse() function, which is only available on Ember 2.0 and above, but since we want to support Ember 1.13 with this addon we used the Walker class instead in the existing AST transform. I have a patch ready here which changes this PR to use the Walker class and will merge it as soon as @marcoow approves this PR too.

@pangratz
Copy link
Contributor

Wohooo! Thanks @HeroicEric!

@Turbo87
Copy link
Collaborator

Turbo87 commented Jan 23, 2017

released as v0.2.0 🎉

@HeroicEric HeroicEric deleted the add-support-for-component-test-selectors branch March 9, 2017 18:17
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