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 "ts" template string handler #122

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
8 participants
@Turbo87
Copy link
Member

Turbo87 commented Jun 23, 2017

... as a replacement for the verbose testSelector() function.

This PR does not yet deprecate the testSelector() function, but replaces it in the README.

Examples

assert.equal(ts`h1`, 'h1');
assert.equal(ts`.foo`, '.foo');
assert.equal(ts`%foo`, '[data-test-foo]');
assert.equal(ts`%foo=1`, '[data-test-foo="1"]');
assert.equal(ts`%foo=bar`, '[data-test-foo="bar"]');
assert.equal(ts`%foo="bar"`, '[data-test-foo="bar"]');
assert.equal(ts`%foo="bar baz"`, '[data-test-foo="bar baz"]');
assert.equal(ts`%foo="bar" baz`, '[data-test-foo="bar"] baz');
assert.equal(ts`%foo='bar' baz`, '[data-test-foo="bar"] baz');
assert.equal(ts`%foo=bar baz`, '[data-test-foo="bar"] baz');
assert.equal(ts`.foo %bar #baz`, '.foo [data-test-bar] #baz');
assert.equal(ts`%primitive-list-header .foo %pointer=3`, '[data-test-primitive-list-header] .foo [data-test-pointer="3"]');
assert.equal(ts`%foo=${'bar'}`, '[data-test-foo="bar"]');
assert.equal(ts`.foo %bar=${5} #bar %foo .${'blubb'}`, '.foo [data-test-bar="5"] #bar [data-test-foo] .blubb');
assert.equal(ts`.foo ${'%bar'}`, '.foo [data-test-bar]');

Resolves #121

/cc @kellyselden

@Turbo87 Turbo87 added the enhancement label Jun 23, 2017

@Turbo87 Turbo87 requested review from marcoow , pangratz and jessica-jordan Jun 23, 2017

find(testSelector('post-title')) // => find('[data-test-post-title]')
find(testSelector('resource-id', '2')) // => find('[data-test-resource-id="2"]')
find(ts`%post-title`); // => find('[data-test-post-title]');
find(ts`%resource-id=2`); // => find('[data-test-resource-id="2"]');

This comment has been minimized.

@cibernox

cibernox Jun 23, 2017

I'd add an example interpolating the id too

This comment has been minimized.

@Turbo87
Add "ts" template literal function
... as a replacement for the verbose testSelector() function

@Turbo87 Turbo87 force-pushed the Turbo87:template-literal-function branch from c12307c to 24083a0 Jun 23, 2017

@rwjblue

This comment has been minimized.

Copy link

rwjblue commented Jun 23, 2017

I don't see why a prefix is needed. What benefit does it provide?

@cibernox

This comment has been minimized.

Copy link

cibernox commented Jun 23, 2017

@rwjblue interpolate testSelectors in longer selectors

this.$(testSelector('post-title')).click() // => this.$('[data-test-post-title]').click()
this.$(testSelector('resource-id', '2')).click() // => this.$('[data-test-resource-id="2"]').click()
this.$(ts`%post-title`).click(); // => this.$('[data-test-post-title]').click();
this.$(ts`%resource-id=2`).click(); // => this.$('[data-test-resource-id="2"]').click();
```

This comment has been minimized.

@jessica-jordan

jessica-jordan Jun 23, 2017

Member

Might it make sense to add a short note on the test selector function here? It could be something along the lines of: If you are using an older version of ember-test-selectors (< 0.4.0), please use the testSelector() [link to a section below with old testSelector syntax examples] function instead of the ts. If you are using a newer version of this addon you are advised to avoid usage of the testSelector function which will be deprecated in the future.

This comment has been minimized.

@Turbo87

Turbo87 Jun 23, 2017

Member

IMHO it should be sufficient to only document the current version of the lib. if people are using older versions of the addon they should look at the README for the version they're using.

This comment has been minimized.

@marcoow

marcoow Jun 26, 2017

Member

If you are using a newer version of this addon you are advised to avoid usage of the testSelector function which will be deprecated in the future.

I think we should keep the testSelector function and see this as a convenience addition only.

@bcardarella

This comment has been minimized.

Copy link

bcardarella commented Jun 23, 2017

This is nice 🎉

@brzpegasus

This comment has been minimized.

Copy link

brzpegasus commented Jun 23, 2017

If the concern is verbosity, we could make things shorter today by doing:

import ts from 'ember-test-selectors';

And then just call ts('selector') instead of testSelector('selector'). Just curious what other benefits I may be missing.

@kellyselden

This comment has been minimized.

Copy link
Contributor

kellyselden commented Jun 23, 2017

@brzpegasus The concern is using multiple to child select, and the best way to build the string #121 (comment)

@marcoow
Copy link
Member

marcoow left a comment

This obviously saves a bunch of characters in many cases but I'd like to make clear we're adding a whole bunch of complexity only for the sake of saving characters which I often find to not be a good tradeoff.

If we add this I'd like us to keep the current testSelector function as well as I personally find

click(testSelector('submit'));

easier to understand then

click(ts`%submit`);

Of course neither testSelector nor ts are really necessary anyway as you could simply write

click('[data-test-submit]');

which might be the best option anyway ;) I guess we should probably change the examples in the README to use plain string selectors like that to make clear what's actually going on and only introduce testSelector and ts as convenience APIs to save some typing and repetition.

if (interpolated.indexOf('%') === -1) {
warn(`[ember-test-selectors] "${interpolated}" contains no test selectors. Please use the % prefix for test selectors (e.g. ts\`%foo\`).`, false, {
id: 'ember-test-selectors.missing-test-selector-prefix',
});

This comment has been minimized.

@marcoow

marcoow Jun 26, 2017

Member

Why is this warning necessary? It's totally fine if it doesn't contain any selectors I'd say. This might be something that would be better implemented in ESLint where people can opt-in/out of it and decide on the warning/error level etc. themselves?

This comment has been minimized.

@Turbo87

Turbo87 Jun 26, 2017

Member

it's not necessary, but can indicate a bug if people forget to include the % prefix. e.g. using ts`submit` instead of ts`%submit`

This comment has been minimized.

@marcoow

marcoow Jun 26, 2017

Member

Yeah, seems like something that could be implemented as an ESLint rule though.

@marcoow

This comment has been minimized.

Copy link
Member

marcoow commented Jul 13, 2017

After thinking about this more I think I'm actually in favor of not merging this and perhaps removing the testSelector helper as well. The whole motivation behind adding that helper and coming up with the entirely new syntax in this PR is so we can reduce the number of chars one has to type to select anything by it's data-test-* attribute. In the case of testSelector though we're just replacing those characters with a function call and in the case of the syntax introduced by this PR we're replacing them with an entirely new syntax that has some shortcomings as discussed above and is also completely new to everyone who already knows CSS selectors.

Neither of the 2 solutions seems worth the cost really just to save a few characters. Also I think it's important to consider that we're not only replacing the actual selector with something else but we're also hiding the selector that will actually be used to identify the element in the DOM. So while all (most) of these examples are slightly longer when typing the complete selector, all of them are easier to follow when using the actual selector instead of something that generates it anyway in the end:

assert.equal(ts`%foo`, '[data-test-foo]');
assert.equal(testSelector('foo', 1), '[data-test-foo="1"]');
assert.equal(ts`%primitive-list-header .foo %pointer=3`, '[data-test-primitive-list-header] .foo [data-test-pointer="3"]');
assert.equal(`${testSelector('primitive-list-header')} .foo ${testSelector('pointer', 3)}`, '[data-test-primitive-list-header] .foo [data-test-pointer="3"]');

Another thing to consider is we haven't even started thinking about more advanced attribute selectors - neither with the testSelector helper nor with %:

find('[data-test-lang|="zh"]') // e.g. either zh-CN or zh-TW
find('data-test-lang~="en-us"') // anything flagged "en-us" (might have more whitespace separated values)
find('data-test-kind^="main"') // anything "main"
@Turbo87

This comment has been minimized.

Copy link
Member

Turbo87 commented Jul 14, 2017

@marcoow I tend to agree, good call

@Turbo87 Turbo87 closed this Jul 14, 2017

@Turbo87 Turbo87 deleted the Turbo87:template-literal-function branch Jul 14, 2017

@rwjblue

This comment has been minimized.

Copy link

rwjblue commented Jul 19, 2017

I agree also, thanks to all involved for thoroughly thinking this through and working out a good result...

@cibernox

This comment has been minimized.

Copy link

cibernox commented Jul 19, 2017

Just to clarify, the general opinion is shifting towards just typing [data-test-foo] instead of using the testSelector('foo') helper? Because it's true that it doesn't save any typing unless you alias it to something very short like ts, and even in that case it is verbose to interpolate.

@marcoow

This comment has been minimized.

Copy link
Member

marcoow commented Jul 19, 2017

Exactly that's the idea. We deprecated the testSelector function and removed it from the readme.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment