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

hover and resize window support #965

Closed
wants to merge 4 commits into from
Closed

Conversation

twalpole
Copy link
Member

This pull request is an attempt at contributing to issue #832 - It implements an api for hover and resize_window. Unfortunately hover doesnt work with firefox on osx (works properly using chrome via webdriver) due to what appear to be positional bugs in selenium

@twalpole
Copy link
Member Author

@abotalov I'm pretty sure it gets reset for every test in Capybara::SpecHelper::reset! - if I'm wrong about that I'll be happy to store and reset the setting

@abotalov
Copy link
Collaborator

  1. I think specs to resize_window and hover shouldn't belong to the same group. For example capybara-webkit currently supports resize_window but doesn't support hover without executing script (at least at first glance, I haven't used it)
  2. @jnicklas If we support set of window size, we should also support get of size. What about alias:
    window_size=(width, height)
    and method:
    window_size returning 2 values: width and height

Also please take a look at Selenium's Window class. Do we need separate class? I think no but want to point it out

@jnicklas
Copy link
Collaborator

Imo, we could combine this with a more sensible alternative to within_window. So we an iterate through open windows, resize them, etc.. The idea of a separate window class is good.

@twalpole
Copy link
Member Author

@jnicklas Moving window related functions to a Capybara::Driver::Window class is fine, but I'm not sure what you're suggesting with within_window. Are you saying that one shouldnt be able to just call resize_window or window.size(xxx,yyy) in the current session to resize the current window?

@@ -203,6 +203,14 @@ def drag_to(node)
synchronize { base.drag_to(node.base) }
end

##
#
# move mouse over the Element
Copy link
Collaborator

Choose a reason for hiding this comment

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

First word should be "Move". Sentence case is used in other comments in this file.

@abotalov
Copy link
Collaborator

@twalpole Please don't add different features in the same pull request next time when you will do it. It will make review easier

@driver=driver
end

def size
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add YARD comments for size and resize?

Copy link
Member Author

Choose a reason for hiding this comment

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

@abotalov There hasnt been agreement on the API yet, I would clean it all up when there is

@twalpole
Copy link
Member Author

@abotalov The only reason they were in the same PR was because they were discussed in the same issue referenced above.

@twalpole
Copy link
Member Author

I dont dealing find dealing with @aboltalov to be worth doing this, closing PR, someone else can add this feature if its wanted

@deivid-rodriguez
Copy link
Contributor

Was this feature ever merged or implemented in any way? Thanks!

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

4 participants