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

Feature/keyboard and mouse actions #12

Merged
merged 13 commits into from
Sep 20, 2019
Merged

Conversation

mialeska
Copy link
Contributor

@mialeska mialeska commented Sep 17, 2019

Add KeyboardActions for Application to close #9
Implement KeyboardActions for element.
implemented MouseActions for custom element and for the whole application to resolve #8
Add localization for new actions.
Add xml documentation to library. Fix documentation for some classes.
Add Scroll as mouse action to resolve #14

@mialeska mialeska self-assigned this Sep 17, 2019
@mialeska mialeska requested a review from hsuboch September 18, 2019 08:43
@mialeska mialeska requested a review from hsuboch September 18, 2019 15:13
@mialeska mialeska requested a review from knysh September 19, 2019 07:54
/// <see cref="OpenQA.Selenium.Keys.Command"/>,
/// <see cref="OpenQA.Selenium.Keys.LeftAlt"/>,
/// <see cref="OpenQA.Selenium.Keys.LeftShift"/>,
/// <see cref="OpenQA.Selenium.Keys.Shift"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you really need this list of Keys in summary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I do. Selenium has it. We either need to follow it's notation, or define a custom Enum with mapping to this values, and accept only them in our methods. I like the first variant more, but we can discuss it

@@ -28,7 +32,7 @@ public override Application Application
var serviceUrl = driverService.ServiceUrl;
localizationLogger.Info("loc.application.driver.service.local.start", serviceUrl);
var driver = GetDriver(serviceUrl, driverSettings.AppiumOptions, timeoutConfiguration.Command);
return new Application(driver, timeoutConfiguration, localizationLogger);
return new Application(driver, timeoutConfiguration, localizationLogger, keyboardActions);
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks very strange. Why do you need to pass actions to the Application?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in order to use the same instance of the keyboardActions, I guess

/// </summary>
/// <param name="key">Keyboard key to define.</param>
/// <returns>Readable value.</returns>
public static string GetLoggableValueForKeyboardKey(this string key)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why it is public? It looks like helper for some library actions, but not as public method that should be available in client code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't see any reason why should we hide the functionality that could be used by the customer.
I can imagine situations when I want to log this value but I cannot.
Seems that I should rework this keys to public enum...

Add xml documentation to library. Fix documentation for some classes.
Add Scroll as mouse action to resolve #14
@mialeska mialeska changed the title Feature/keyboard actions Feature/keyboard and mouse actions Sep 19, 2019
@mialeska mialeska requested a review from paveliam September 19, 2019 13:16
@mialeska mialeska merged commit 43f86a9 into master Sep 20, 2019
@paveliam paveliam deleted the Feature/Keyboard-Actions branch May 7, 2020 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants