Skip to content
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

[Feature] Add WaitForClickable helper #635

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JoppeDC
Copy link

@JoppeDC JoppeDC commented Jul 25, 2024

No description provided.

* @throws NoSuchElementException
* @throws TimeoutException
*/
public function waitForClickable(string $locator, int $timeoutInSecond = 30, int $intervalInMillisecond = 250): PantherCrawler
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$timeoutInSecond -> $timeoutInSeconds ?

same for $intervalInMillisecond -> $intervalInMilliseconds ?

Copy link
Author

@JoppeDC JoppeDC Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this was a little weird too, but this is the way its done for all the other cases as well.

Cant change it for the other cases either, as thats a BC. We keep it like this so they're the same, or adjust this one?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. The same arguments in other methods are indeed named in the same way you did.

Changing the argument names can be made in other PR if wanted. By the way, Symfony's BC promise doesn't cover argument names, so we can change them if we decide it's better to do so. See https://symfony.com/doc/current/contributing/code/bc.html#note-10

Thanks

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, TIL!

I think its best to change all the names in a seperate PR in that case :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants