feat: add locator functionality (#238) #267
Conversation
There was a problem hiding this comment.
@masaushi you did an amazing job there, I really appreciate your time and effort which you put into this! 💯
Thank you for contributing to the project, all the comments are just small nits + the patch files seem broken, maybe out of date, I can take a look at it if you struggle let me know!
frame.go
Outdated
| } | ||
|
|
||
| func (f *frameImpl) queryCount(selector string) (int, error) { | ||
| channel, err := f.channel.Send("queryCount", map[string]interface{}{ |
There was a problem hiding this comment.
| channel, err := f.channel.Send("queryCount", map[string]interface{}{ | |
| response, err := f.channel.Send("queryCount", map[string]interface{}{ |
| var option *LocatorLocatorOptions | ||
| if len(options) == 1 { | ||
| option = &options[0] | ||
| if option.HasText != nil { |
There was a problem hiding this comment.
in other typed language bindings (java, dotnet) we introduced options.HasTextRegex and options.HasTextString. But I think its fine for us to keep it like you did.
locator.go
Outdated
| return result, nil | ||
| } | ||
|
|
||
| func (l *locatorImpl) BoundingBox(options ...LocatorBoundingBoxOptions) (*LocatorBoundingBoxResult, error) { |
There was a problem hiding this comment.
| func (l *locatorImpl) BoundingBox(options ...LocatorBoundingBoxOptions) (*LocatorBoundingBoxResult, error) { | |
| func (l *locatorImpl) BoundingBox(options ...LocatorBoundingBoxOptions) (*Rect, error) { |
locator.go
Outdated
| strict := true | ||
| options[0].Strict = &strict |
There was a problem hiding this comment.
| strict := true | |
| options[0].Strict = &strict | |
| options[0].Strict = Bool(true) |
I think we have a wrapper function for that.
There was a problem hiding this comment.
I didn't know that useful wrapper function!
fixed codes including similar to this!
locator.go
Outdated
| } | ||
|
|
||
| var result interface{} | ||
| err := l.withElement(func(handle ElementHandle) (err error) { |
There was a problem hiding this comment.
in theory we could change withElement that it returns (error, interface{}) so we don't need to have the outer variable assignment. Not sure tho if thats better!
There was a problem hiding this comment.
yes, your suggestion is right.
I changed withElement to return (interface{}, error)!
locator.go
Outdated
| func (l *locatorImpl) Hover(options ...PageHoverOptions) error { | ||
| if len(options) == 1 { | ||
| strict := true | ||
| options[0].Strict = &strict |
There was a problem hiding this comment.
ditto etc. as per above with the Bool(true) comment.
tests/locator_test.go
Outdated
| require.ElementsMatch(t, []string{"A", "B", "C"}, innerHTML) | ||
| } | ||
|
|
||
| func TestLocatorAllTextContens(t *testing.T) { |
There was a problem hiding this comment.
| func TestLocatorAllTextContens(t *testing.T) { | |
| func TestLocatorAllTextContents(t *testing.T) { |
|
@mxschmitt |
|
Amazing, bots are green and that means your patch related changes were good! |
|
|
||
| func (l *locatorImpl) Fill(value string, options ...FrameFillOptions) error { | ||
| if len(options) == 1 { | ||
| options[0].Strict = Bool(true) |
There was a problem hiding this comment.
This forcefully overwrites whatever config that is passed in unless they pass 2 configs in. Is that intentional?
This is implementation for
Locator(#238).Please review the implementations.
thank you!