add match exact text support #488
Conversation
|
Thanks for opening PR! I agree that the issue should be addressed. There are few quick implementation related observations:
|
|
@ro0gr thanks for reviewing agreed for the first point, even I thought the same but since this package already uses jQuery thought of going with the flow :P here is a quick & rough implementation https://jsfiddle.net/s1ywa0dz/1/ for point 2 don't u think its necessary elsewhere? |
That's true. However, we intend to reduce dependency on jQuery or even remove it at some point. So if we can avoid new integration points, let's avoid it.
No. Sorry, my statement must be confusing. I don't think we need One more point. that seems to be important. I'm currently working on initial refactoring and preparations for the v2. It significantly simplifies the properties, and |
|
@ro0gr IDK whether it makes sense, I have faced a scenario with one of the packages we use at work. Since upgrading a larger codebase is a bit of pain in the a** but the support for a certain feature was provided only in the latest release of the package since I recently started contributing to OS I am not sure how this works, but wouldn't it be great to support the older version as well? especially in the case where haven't yet release v2 and the fact that it may take time |
I'm not sure to which feature do you refer. But assuming you are talking about Initially I thought, just providing codemods for finders would make migration easy enough. But if consider a large codebase, which has addons with their own page objects, I can see how such a migration can become painful. I think I'll make a step back in the beta branch and restore these 2 utils with deprecations prolongated until v3, but w/o Coming back to where to implement the feature. I'm not opposed to add it to the v1. It'd just take some extra effort to make it a part of the v2, since all the actions are going to be re-written slightly. Though, it should not be a big issue, and if you need it soon - go for it. |
|
@ro0gr + 1, so you are saying if I made necessary changes in this PR, we can merge? edit: also are we planning to remove jQuery completely in v2? |
yes.
There is no such a strict plan. I'd say it would be nice to have an ability to opt-out jQuery in scope of v2 on user permise, in order to avoid a hard dependency on jQuery selectors API. |
mum-never-proud
force-pushed
the
add-match-exact-support
branch
from
71e58d1 to
7776cbe
Compare
|
I have fixed the exact option leak but for a custom implementation of the pseudo selector, I guess we can go with this approach before starting $(selector, container) {
if (container) {
return $(selector, container);
} else {
// @todo: we should fixed usage of private `_element`
// after https://github.com/emberjs/ember-test-helpers/issues/184 is resolved
let testsContainer = this.testContext ?
this.testContext._element :
'#ember-testing';
return $(selector, testsContainer);
}
}my proposal is to check the selector with regex your thoughts? cc @ro0gr |
|
@mum-never-proud I'm not sure. I'd like us to be able to add such a feature by changing only I think ideally, we should be able to lookup an element to click in the action, and then pass it to the interaction method, so we don't enforce the rest of the system to know about the custom selectors just for one case scenario. So, in my opinion, instead of (from v1): or (from v2-beta); we need a way to pass the element instead of combination of pseudo-code: if (options.matchExact) {
const els = findMany(this, selector, options).filter(el => $(el).text() === textToClick);
guardMultiple(els);
// also needs to handle contextual error somehow
return click(els[0]);
}smth like that still should be possible to achieve in v2(sorry for shifting to that direction), I'll take this use case into a consideration. But for v1 it doesn't seem possible or easy to achieve, cause the current execution contexts do not accept HTML elements to actions. Don't have a clear path forward right now, let me think a bit more about it please. |
|
lately I was just brainstorming a bit on this approach
from my understanding, we make use of assertElementExists(selector, options) {
/* global find */
let result = find(selector, options.testContainer || findClosestValue(this.pageObjectNode, 'testContainer')).toArray();
Object.values(getCustomTextFilters(options))
.forEach(customFilter => {
result = result.filter($ele => customFilter($ele.textContent.trim(), options.contains));
});
if (result.length === 0) {
throwBetterError(
this.pageObjectNode,
options.pageObjectKey,
ELEMENT_NOT_FOUND,
{ selector }
);
}
return result;
}I think while asserting we must also return the element to reduce one round of querying dom, its my understanding that if we pass direct DOM Node jQuery doesn't query. Object.values(getCustomTextFilters(options))
.forEach(customFilter => {
result = result.filter($ele => customFilter($ele.textContent.trim(), options.contains));
});this one pretty straight forward we get the user given custom text filters loop and filter out the element the tests are passing and I don't see any regression happening, I am not sure if we are lacking tests I think this general approach can be extended without depending on jQuery still, I am not much familiarized with all of the codebase I might have missed a few edge cases. let me know your thoughts or if you need further clarification :) tanks! cc: @ro0gr |
|
|
||
| return context.click(fullSelector, container, options); | ||
| return context.click($ele[0], container, options); |
There was a problem hiding this comment.
This is technically a breaking change, unfortunatelly. At the moment execution contexts are a bit inconsistent. Some of them(RFC268, NativeEvents), do make sure they do only operate with a single element,
while the rest(Acceptance, Integration) just accept a selector and then pass it to a certain action helper.
you can compare it by looking at
and
While I believe actions against multiple elements is bad, I think we can't just break this behavior w/o major version bump. And that's one of the primary point for v2(#479).
At the moment, the best way I can think of to achieve that behavior in v1 may be to introduce a new click(clickElement?) method for each execution context, which would accept just an element. It would also require a special code path in the click-on-text itself for matchExact only. And that all would be removed in favour of a more concise handling of matchExact in v2.
thoughts?
yes, but we aim to remove it in favour of |
Resolves #272