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

Added support for event listeners for actions done on elements provided by SeLion. #23

Closed
wants to merge 1 commit into from

Conversation

renelux
Copy link
Contributor

@renelux renelux commented Sep 16, 2014

Adds support to SeLion to trigger events before and after we perform actions on SeLion elements. This will help us to make SeLion more extendable.

The following events are supported:

Click
Type
Check
Uncheck
Submit
Select
Deselect

@mach6 mach6 added this to the M3 milestone Sep 19, 2014
@mach6
Copy link
Contributor

mach6 commented Sep 26, 2014

@fakrudeen78 @krmahadevan @sebady I'm inclined to merge this. Do any of you have feedback on this PR? I know there was talk about accomplishing this a different way. Do we still want to pursue that?

@sebady
Copy link

sebady commented Sep 29, 2014

This approach for supporting event listeners looks reasonable to me. I'm not aware of an alternative under consideration/discussion. I would merge it.

@krmahadevan
Copy link

I haven't yet had the chance to revisit this yet. Been swamped with stuff. I would want to look at this again since I was involved in the original discussions and first set of review on this.

  • Cheers
    Krishnan.

Sent from a mobile.
So if you noticed any typos you know whom to blame 😊

-------- Original message --------
From: Sherif Ebady notifications@github.com
Date:09/29/2014 20:16 (GMT+05:30)
To: paypal/SeLion SeLion@noreply.github.com
Cc: "Mahadevan, Krishnan" krmahadevan@paypal.com
Subject: Re: [SeLion] Added support for event listeners for actions done on elements provided by SeLion. (#23)

This approach for supporting event listeners looks reasonable to me. I'm not aware of an alternative under consideration/discussion. I would merge it.


Reply to this email directly or view it on GitHubhttps://github.com//pull/23#issuecomment-57171623.

@krmahadevan
Copy link

@mach6
@fakrudeen78

Guys I am fine with this implementation being merged.

But we need to take care of the below mentioned trivial things before the code hits develop

  • In com.paypal.selion.platform.grid.AbstractTestSession.getListeners() We can consider renaming this to getSeLionEventListeners() to make sure the intent of this method is clear.
  • In com.paypal.selion.platform.grid.ScreenShotRemoteWebDriver.register(SeLionElementEventListener) -
    Please add a Null Check here, because this is attempting to add an element to the arraylist and I dont think array list does a null check (as the List contract states).
  • In com.paypal.selion.platform.html.support.events.AbstractSeLionElementEventListener This class should be an abstract class rather than a concrete class so that it is in sync with what org.openqa.selenium.support.events.AbstractWebDriverEventListener works as in principle.
  • In the package com.paypal.selion.platform.html.support.events all the interfaces need some basic javadocs which convey the intent of these marker interfaces.
  • We need to clean up the un-used imports in com.paypal.selion.testcomponents.BasicPageImplTest.

@mach6
Copy link
Contributor

mach6 commented Oct 8, 2014

@krmahadevan @fakrudeen78 @renelux I've pulled this contribution into the branch https://github.com/paypal/SeLion/tree/renelux-patch-add-events so that we can address the review comments. Furthermore, I have rebased it off of the latest https://github.com/paypal/SeLion/tree/develop branch

@mach6
Copy link
Contributor

mach6 commented Oct 14, 2014

I also changed all references of SeLionElementEventListener to ElementEventListener. I don't see the need for the use of the word SeLion since 1. There is no ElementEventListener's in Selenium code (that I see) and 2. the word selion is already in the package.

mach6 pushed a commit that referenced this pull request Oct 14, 2014
mach6 pushed a commit that referenced this pull request Oct 15, 2014
mach6 pushed a commit that referenced this pull request Oct 17, 2014
@mach6
Copy link
Contributor

mach6 commented Oct 17, 2014

This is now merged to develop. Issue #38 opened to track the documentation needs for this PR.

@mach6 mach6 closed this Oct 17, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants