Changed find_by_name to find_by_xpath in fill() #1100
Conversation
When dealing with a page where the name is set dynamically we were not able to use fill() or attach_file() as it was accepting name as a parameter, so changed it to xpath as we can always get a xpath which is unique
| def fill(self, xpath, value): | ||
| field = self.find_by_xpath(xpath).first |
There was a problem hiding this comment.
This would be a breaking change and would make the API inconsistent, so it's unlikely that we'll merge it. We could introduce something like fill_by_xpath, but I'd like to hear from others before we move forward with an addition to the API.
@andrewsmedina @jsfehler any thoughts?
There was a problem hiding this comment.
I think that's the wrong direction. I'm in favour of removing browser.fill() entirely.
browser.fill("foo", "bar") is always going to be difficult to read. It's too opaque.
browser.find_by_xpath("foo").fill("bar") is easier to read and more explicit.
If we want an implicit find function, it should look like:
browser.find("foo").fill("bar")
browser.find(xpath="foo").fill("bar")
It should default to CSS, accept other strategies as arguments, and be configurable to default to other strategies.
There was a problem hiding this comment.
I agree with @jsfehler the solution i gave looks temporary and it is also opaque, we should implement something like browser.find_by_xpath("foo").fill("bar") and same for attach_file()
|
With #933 merged in, I don't see this PR as necessary anymore. |
When dealing with a page where the name is set dynamically we were not able to use fill() or attach_file() as it was accepting name as a parameter, so changed it to xpath as we can always get a xpath which is unique