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

Strip "data-test-" attributes from component and helper invocations #40

Merged
merged 4 commits into from
Jan 9, 2017

Conversation

Turbo87
Copy link
Collaborator

@Turbo87 Turbo87 commented Jan 9, 2017

This automatically removes data-test- prefixed attributes from component/helper invocations in templates like {{some-component data-test-name="foobar"}}. This is roughly equal to what we've already been doing on vanilla HTML tags like <div data-test-name="foobar">.

These tests are only used to check the stripping on HTML tags, not components.
This conflicts with using a `{{data-test}}` property inside a component template
@@ -0,0 +1,4 @@
<div class="attr">{{data-test-attribute}}</div>
Copy link
Member

Choose a reason for hiding this comment

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

This is an example of using a data-test-* attribute in the wrong place actually. It should be stripped but should actually never appear here in the first place.

Copy link
Contributor

@pangratz pangratz Jan 9, 2017

Choose a reason for hiding this comment

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

This is only used for testing purposes to see that the data-test-attribute is correctly stripped and nothing is rendered within the div...

});

if (config.stripTestSelectors) {

Copy link
Member

Choose a reason for hiding this comment

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

This needs a test that while

{{my-component data-test-attr='value'}}

becomes

<div class="ember-view" data-test-attr="value"></div>

in environments that we don't strip attributes for it becomes

<div class="ember-view"></div>

when data-test-* attributes are stripped.

this.render(hbs`{{print-test-attributes data-non-test="foo"}}`);

assert.equal(this.$('.non-test').text(), 'foo', 'the .non-test div is not empty');
});
Copy link
Member

Choose a reason for hiding this comment

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

These cases actually all test edge cases. I guess that's fine but they all assume wrong usages of data-test-* attributes.

@marcoow
Copy link
Member

marcoow commented Jan 9, 2017

This should also test that data-test-* attributes in a component's context are stripped, e.g.

export default Component.extend({
  'data-test-my-attr': computed(function() {
    return 'whatever';
  }),

@@ -0,0 +1,4 @@
<div class="attr">{{data-test-attribute}}</div>
Copy link
Contributor

@pangratz pangratz Jan 9, 2017

Choose a reason for hiding this comment

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

This is only used for testing purposes to see that the data-test-attribute is correctly stripped and nothing is rendered within the div...

<div class="attr">{{data-test-attribute}}</div>
<div class="second">{{data-test-second}}</div>
<div class="non-test">{{data-non-test}}</div>
<div class="data-test">{{data-test}}</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the class names in here should be named consistently to reflect the corresponding attribute. I would suggest the class name to be the same name as the attribute, so we have .data-test-attribute, .data-test-second, .data-non-test and .data-test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pangratz good idea. done.

@Turbo87
Copy link
Collaborator Author

Turbo87 commented Jan 9, 2017

After a short voice conversation we've resolved a small misunderstanding about the purpose of this PR and the tests related to the print-test-attributes component.

tl;dr this PR is focused only on the stripping of data-test- attributes from component/helper invocations and the corresponding tests for that feature

@Turbo87 Turbo87 merged commit 238da86 into mainmatter:master Jan 9, 2017
@Turbo87 Turbo87 deleted the strip-from-components branch January 9, 2017 11:57
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.

3 participants