fix: Revert native contains property #547
Conversation
The change done in this PR: san650#466 caused a regression. $.text() can find the text in `visibility: hidden` or `display: none` elements. element.innerText does not return the text. Fixes: san650#514
| import { findMany, findOne } from '../extend'; | ||
| import { A } from '@ember/array'; | ||
| import { assign, every } from '../-private/helpers'; | ||
| import { findElementWithAssert } from '../extend'; |
There was a problem hiding this comment.
internaly we tend to avoid using findElementWithAssert( in favour of findMany or findOne. I'd appreciate if we can avoid findElementWithAssert here as well.
There was a problem hiding this comment.
Done. However because findMany and findOne return a normal array and not a jQuery Array I needed to convert the elements into a jQuery array for use in the every helper function.
There was a problem hiding this comment.
in order to avoid custom every util, we could just wrap each element with $, like instead of
return A(elements).every((element) => element.innerText.indexOf(textToSearch) >= 0);we could have
return A(elements).every((element) => $(element).text().indexOf(textToSearch) >= 0);Though, I think it isn't that important.
There was a problem hiding this comment.
Hmm I tried this but the every util helper needs a jquery array to be passed in
This keeps the helper in line with other internal helpers. This required converting the e lements into a jQuery array as this is what the `every` function receives.
|
@ro0gr I added tests and updated the helper to use findMany and findOne I would love help getting this merged and released as soon as you can :) It is blocking me on a bug in one of the apps that I maintain. Thank you so much for this addon and for your continued maintenance! 🥇 |
|
@Alonski sure! I'm going to publish it tonight |
|
So turns out it doesn't really fix the #514 (comment), which has never been a regression. But it fixes a real regression of the |
|
As a side note, seems Travis does not work for few month already and requires some migration procedure. So I've checked your tests locally, and it works on my end 🟢 @san650 would you be able to migrate this repo to the new Travis service? - https://docs.travis-ci.com/user/migrate/open-source-repository-migration#migrating-a-repository, cause seems I'm not allowed to do this. It'd be awesome. |
|
published as 1.17.8 |
|
@ro0gr thanks for the heads up. I've migrated the repo to use travis-ci.com https://app.travis-ci.com/github/san650/ember-cli-page-object/requests?requestId=625350776 |
|
@san650 Awesome. Thank you! |
|
@ro0gr It seems that now I added a regression in this PR 🙃 I added a dependency to the I need to think of a way to fix this as we also can't use |
|
@Alonski hm.. that's strange. ec-page-object is supposed to use it's own jquery instance if there is no one provided by a host. It doesn't sound related to your change to me. Do you have a chance to share a reproduction, and maybe create a bug ticket? |
|
@ro0gr Created a reproduction in StackBlitz: In console: npm i
npm run test:ember --server
|
| import { findMany, findOne } from '../extend'; | ||
| import { A } from '@ember/array'; | ||
| import $ from 'jquery'; |
There was a problem hiding this comment.
oh... I must be blind.
At the moment in order to use "adaptive" jquery, we had to import it by a synthetic -jquery path, like in other properties
There was a problem hiding this comment.
Do you want to create a PR to fix this or want me to whip one up?
There was a problem hiding this comment.
I can do it, though not sure if I manage to handle it today
There was a problem hiding this comment.
so yes, this error pops up in the GH Actions PR #552. and it's fixed there as well. I'd fix it altogether this weekend.
|
@ro0gr Can you please also add the |
The change done in this PR: #466 caused a regression.
$.text() can find the text in
visibility: hiddenordisplay: noneelements.element.innerText does not return the text.
Fixes: #514