-
-
Notifications
You must be signed in to change notification settings - Fork 833
Conversation
2e8e474
to
1645ee8
Compare
@weeman1337 I opened a PR by using the library (the library looks great!), and I am just wondering if it could have more than one argument, such as: |
PR welcome :) I would suggest to just pass all options through to |
Cypress.Commands.add("findButton", (name: string): Chainable<JQuery> => { | ||
return cy.findByRole("button", { name }); | ||
}); | ||
|
||
Cypress.Commands.add("findTextbox", (name: string): Chainable<JQuery> => { | ||
return cy.findByRole("textbox", { name }); | ||
}); | ||
|
||
Cypress.Commands.add("findOption", (name: string): Chainable<JQuery> => { | ||
return cy.findByRole("option", { name }); | ||
}); | ||
|
||
Cypress.Commands.add("findMenuitem", (name: string): Chainable<JQuery> => { | ||
return cy.findByRole("menuitem", { name }); | ||
}); |
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.
@weeman1337 I think I'm missing the value of these helper functions.
As someone relatively new to the react-sdk codebase and cypress, and completely new to the cypress testing library, I spent a while trying to find documentation online these helper functions: I didn't realise that they were helpers that we had defined ourselves.
Given they are so short, why not just inline them?
In other words: I feel like these helpers are making our codebase less accessible to new contributors, whilst doing little to simplify the code.
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 don't have a strong opinion on them. My intention was that „I want to find a button“ by findButton("Submit")
is probably better understandable than findByRole("button", { name: "Submit" })
.
@richvdh would you like to raise this in one of our discussions? We can then decide whether to inline and remove them.
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.
Just for the record: we are going to remove them; it's cleaner and more flexible just to call cy.findByRole
directly.
Task from one of our recent weeklies.
findButton(name)
PSF-1968
Checklist
This change is marked as an internal change (Task), so will not be included in the changelog.