Skip to content

Add rails5 support#198

Merged
magneland merged 24 commits intomasterfrom
hexAddRails5Support
Jul 4, 2017
Merged

Add rails5 support#198
magneland merged 24 commits intomasterfrom
hexAddRails5Support

Conversation

@ipmsteven
Copy link
Copy Markdown
Contributor

No description provided.

window.location.href = event.target.getAttribute("data-href");
}, 3000);
})
}) No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

MIssing newline.

end
end

class Rails5 < Rails4; end
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps instead of subclassing, we could rename this class to reflect that it is for Rails 4+?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@jonkessler
I was thinking about that but failed to make a good name :(
any suggestion for naming?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rails4Plus?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍


def test_window_change_to__multiple_pages
book = Book.create!(:title => "Brave New World")
book = Book.create!(:title => 'Brave New World', :author => Author.create!(:last_name => 'Huxley'))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could use new hash syntax in these two spots, since we're changing this.

@ipmsteven ipmsteven force-pushed the hexAddRails5Support branch from c596fb1 to 82f9c9a Compare June 29, 2017 17:05
Copy link
Copy Markdown
Member

@magneland magneland left a comment

Choose a reason for hiding this comment

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

Looks good overall.

Can you also update this sentence in README.md:
AePageObjects is built to work with Rails (versions 3.X-4.X) out of the box.

Can you also update this sentence in the development.md:

`AePageObjects::ApplicationRouter` is tested against various versions of Rails in the _test/test_apps_ directory (currently 3.0 to 4.2).

case(RUBY_VERSION)
when "2.2.5" then

appraise "capybara-2.2-ruby#{RUBY_VERSION}" do
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.

So many capybaras...are they all necessary?
On the other hand, there are newer versions as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So many capybaras...are they all necessary?

I think we did it on purpose base on this https://github.com/appfolio/ae_page_objects/blob/master/development.md#integration

On the other hand, there are newer versions as well.

yea, maybe we should have separate PR for adding new capybara support

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.

Added a card

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

👍

@ipmsteven ipmsteven force-pushed the hexAddRails5Support branch from 4c7078a to 39f777b Compare July 3, 2017 23:01
@magneland magneland merged commit 5567d38 into master Jul 4, 2017
@magneland magneland deleted the hexAddRails5Support branch July 4, 2017 00:58
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