search

uezo / TinySeleniumVBA

A tiny Selenium wrapper written in pure VBA
MIT License
60 stars 16 forks source link

Some Improvements #3

Closed tdmsoares closed 3 years ago

tdmsoares commented 3 years ago

Fixed driverUrl Fixed driverUrl value in Sub Chrome(ByVal driverPath As String, Optional ByVal driverUrl As String) The default value of driverUrl in this Sub was set to "http://localhost:9151" instead of "http://localhost:9515" which was causing an error while trying to start Chrome browser To prevent this, it was created a Const DRIVER_URL with the right address to WebDriver and, as this address is the same with both EdgeDriver and ChromeDriver and can be used as the default value of Sub Chrome and Sub Edge Enabled FindElement by Xpath Added code to enable FindElement by Xpath solving the Issue #2

uezo commented 3 years ago

Hi @tdmsoares , thank you for your fantastic contribution! Please give me separated PR for each theme if you don't mind.🙇‍♀️

Fixed driverUrl

First, 9151 was typo 🙃 I'm sorry for confusing. However we should consider more about the idea to define the default driver url as Const because the url is different the one of ChromeDriver(9515) from GeckoDriver(4444). I think setting driver url as the default value of argument is more explicit way to show that difference. If you agree with this idea will you give me the PR to fix the port number from 9151 to 9515?

XPath

It looks good to me! Will you remove some "'" (single quotation) lines that doesn't have any comment like below? https://github.com/uezo/TinySeleniumVBA/pull/3/commits/62893ae8f54f4256d451fd183037bbe1da29c1ed#diff-8164268258b8efc2df2695f6b942ab032b48226ae8b0964dc994597a22ccf92cR319

Typo

I'm sorry...Thank you for fix👍

tdmsoares commented 3 years ago

Hi @uezo I tried to create a different PR for each of changes. This is new for me (not so used with github), tried git cherry-pick in a new branch, I don't know I did it right (In the case you can tell me please). Well, at least I learned that the next time I have to create a new branch before making changes...

About the title Fixed driverUrl I think you're right, specially if another Webdriver has a different URL, it's better not use the same Const assuming it is an universal Url for all drivers: I removed and placed the default URL in the function parameters

Relating to XPath I removed the single quotes in the blank lines (sometimes I use this because it gives me more readability on the code), but no problem. Another thing: I had to make two commits about the same topic Xpath because the first I added the code in the function ToSelector but I forgot to save with the Xpath element in enum By, solved in the second commit

And with the Typo, don't worry, not a big deal. And I have to assume I also make mistakes, no problem :)

tdmsoares commented 3 years ago

Hi @uezo I made 4 PRs for each theme as you asked. Each PR relates to one branch, with one functionality. So I'll close this PR because it contains several different changes on the same PR corresponding to the main branch