-
Notifications
You must be signed in to change notification settings - Fork 154
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
feat(within): Support cy.within #11
Conversation
…within the options object. * Installed wait-port and use it within test:cypress:run to avoid race-conditions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Would be good to get another review though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall, but I'd make some adjustments to make things clearer.
|
||
it('getByText in container', () => { | ||
cy.get('#nested') | ||
.then((subject) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear from the documentation https://docs.cypress.io/api/commands/then.html#DOM-element , but as I remember, .then
callback gets a jQuery collection in its first argument, not a single DOM element. Could you please verify this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, you use Cypress.dom.isJquery(element)
to accept both jQuery collection and a single element as the container. This wasn't the case before, so likely requires a documentation update where the container
option type is described.
"test:cypress:serve": "serve -l 13370 ./cypress/fixtures/test-app", | ||
"test:cypress:run": "cypress run", | ||
"test:cypress:serve": "serve --listen 13370 ./cypress/fixtures/test-app", | ||
"test:cypress:run": "wait-port --timeout 10000 localhost:13370 && cypress run", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
src/support/utils.js
Outdated
return element | ||
} | ||
|
||
function getContainer(cy, container) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cy
is assumed a global in Cypress, too, so either make this function pure and take both Cypress
and cy
from arguments (and pass Cypress
to getElement
), or just take both from globals.
src/support/utils.js
Outdated
@@ -0,0 +1,21 @@ | |||
function getElement(element) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function getFirstElement(jqueryOrElement) {
would state the intent clearer.
src/index.js
Outdated
@@ -1,4 +1,5 @@ | |||
import {queries, waitForElement} from 'dom-testing-library' | |||
import {getContainer} from './support/utils' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why did you place this file as ./support/utils
— the file structure of ./
is not dictated by Cypress, so it's confusing as if the structure mattered for Cypress; just ./utils
is enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I remembered you already had a utils file within your code from #6:
import {
makeCypressCommandLog,
makeElementDebugString,
getNodeFullText,
} from '../support/utils';
So I just tried to match that 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The snippet I have shown is in the cypress
folder of my app where Cypress defines the file structure, it's not in the library code.
* Documented jQuery vs DOM nodes. * Use Cypress.cy global. * Renamed function and parameter. * Moved utils file.
@npeterkamps Thanks for the fixes in 6394e7e – I apologize for the confusion that I brought, but I did not notice that I wonder why |
Yes. The only API that
Since Cypress uses jQuery elements and |
Thanks @npeterkamps, all looks good. |
Thanks friends! |
🎉 This PR is included in version 2.2.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
@kentcdodds I didn't find this in |
I think it's in the maintaining.md in the |
@kentcdodds Found it, thanks! |
What / How:
cy.within
by usingcy.state('withinSubject')
when available.dom-testing-library
expects DOM nodes, created a helper to retrieve the DOM node.instanceof
didn't work.test:cypress:run
to avoid race-conditions.Why:
See #8
Checklist: