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

Minor rewordings #455

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

ThomasLandauer
Copy link
Contributor

No description provided.

@@ -45,8 +45,6 @@ Use [Composer](https://getcomposer.org/) to install Panther in your project. You

### Installing ChromeDriver and geckodriver

Panther uses the WebDriver protocol to control the browser used to crawl websites.
Copy link
Member

Choose a reason for hiding this comment

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

Why should we delete this? It's an important information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A quick Ctrl+F for "webdriver" gives me:

  • It leverages the W3C's WebDriver protocol
  • $client->getWebDriver()->manage()->getLog('browser');
  • client->getWebDriver()->manage()->getLog('performance');
  • Checking the State of the WebDriver Connection
  • Use the Client::ping() method to check if the WebDriver connection is still active
  • because this library uses WebDriverElement internally
  • Panther is built on top of PHP WebDriver

So if you merge this, it will still be explained 7 times :-)

Copy link
Member

Choose a reason for hiding this comment

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

It's stated only one time that it's a W3C spec and it's an important information (especially because there are several things named WebDriver).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I don't see your point.

It's stated only one time that it's a W3C spec

Well, but the sentence I deleted doesn't contain that! ;-)
Besides, the "installation" section just isn't the right place to inform people what this project is all about. This should be explained at the very top, and the third sentence on the page does this very clear IMO:

It leverages the W3C's WebDriver protocol to drive native web browsers such as Google Chrome and Firefox.

README.md Outdated
and [GeckoDriver](https://github.com/mozilla/geckodriver) (for Firefox) and put them anywhere in your `PATH`
or in the `drivers/` directory of your project.
Finally, you can manually download [ChromeDriver](https://sites.google.com/a/chromium.org/chromedriver/) (for Chromium or Chrome)
or [GeckoDriver](https://github.com/mozilla/geckodriver) (for Firefox) and add them to your `PATH` or put them
Copy link
Member

Choose a reason for hiding this comment

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

and is better (you may need both).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed it to "and/or", since not everybody needs both. OK?

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.

None yet

2 participants