Skip to content

added scroll distance support for iOS's scroll script execution.#805

Closed
saim80 wants to merge 4 commits intoappium:masterfrom
untitledstartup:mighty
Closed

added scroll distance support for iOS's scroll script execution.#805
saim80 wants to merge 4 commits intoappium:masterfrom
untitledstartup:mighty

Conversation

@saim80
Copy link
Copy Markdown

@saim80 saim80 commented Jul 30, 2018

No description provided.

@jsf-clabot
Copy link
Copy Markdown

jsf-clabot commented Jul 30, 2018

CLA assistant check
All committers have signed the CLA.

Copy link
Copy Markdown
Member

@KazuCocoa KazuCocoa left a comment

Choose a reason for hiding this comment

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

Thanks!
I commended rubocop issues.
Could you sign the CLA?

# scroll direction: "down"
# ```
def scroll(direction:, name: nil, element: nil, to_visible: nil, predicate_string: nil)
def scroll(direction:, distance: nil, name: nil, element: nil, to_visible: nil, predicate_string: nil)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you add # rubocop:disable Metrics/ParameterLists at the end of this line to avoid rubocop?

end

# @param [string] direction Either 'up', 'down', 'left' or 'right'.
# @option opts [Double] :distance scroll distance, proportional to scroll view height.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Remove a white space here to fix rubocop

# scroll direction: "down"
# ```
def scroll(direction:, name: nil, element: nil, to_visible: nil, predicate_string: nil)
def scroll(direction:, distance: nil, name: nil, element: nil, to_visible: nil, predicate_string: nil) # rubocop:disable Metrics/ParameterLists
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rubocop pointed out another issue... Could you fix it like https://github.com/appium/ruby_lib_core/blob/21620568f14ed4537b43c29e2b28026795e30617/lib/appium_lib_core/common/device/image_comparison.rb#L105-L111 ?

You can run rubocop with rake rubocop on your local, btw.

thanks for the contribution

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I modified the line as suggested. I ran the rake rubocop I see other warnings as well when I ran the command, should I be worried?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ah, yeah. Rubocop raises warnings depends on its versions.
you can run it with bundle exec rake rubocop to specify the version.

# scroll direction: "down"
# ```
def scroll(direction:, name: nil, element: nil, to_visible: nil, predicate_string: nil)
def scroll(direction:,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The below should make rubocop 💚

        def scroll(direction:, # rubocop:disable Metrics/ParameterLists
                   distance: nil,
                   name: nil,
                   element: nil,
                   to_visible: nil,
                   predicate_string: nil)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@saim80
Hi, do you have a chance to fix the rubocop?
(If you have, I'd like to include this in next release with #806 )

KazuCocoa added a commit to KazuCocoa/ruby_lib that referenced this pull request Aug 9, 2018
@KazuCocoa KazuCocoa mentioned this pull request Aug 10, 2018
KazuCocoa added a commit that referenced this pull request Aug 10, 2018
* fix rubocop warning for #805

* update changelog

* add a link to the user
@KazuCocoa
Copy link
Copy Markdown
Member

@saim80
I appreciate you for this PR!
But I'd like to publish a new version including this change. So, I created another PR https://github.com/appium/ruby_lib/pull/809/files and have merged it.
I mentioned you https://github.com/appium/ruby_lib/blob/master/CHANGELOG.md#1-enhancements

Thanks again!

@KazuCocoa KazuCocoa closed this Aug 10, 2018
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.

3 participants