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

Rails 4 2 #38

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Rails 4 2 #38

wants to merge 20 commits into from

Conversation

jedschneider
Copy link
Member

updating gem to support Rails 4.2 and modern versions of development libraries like Cucumber and RSpec

@@ -126,6 +126,7 @@
When /^I change the slug to "([^"]*)"/ do |path|
fill_in 'Slug', :with => path
click_button 'Update Page'
page.should_not have_content('Update Page')

Choose a reason for hiding this comment

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

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@jedschneider
Copy link
Member Author

@jejacks0n if you have a few I wanted to just run a couple things by you regarding mercury and teaspoon.

  • In ea5c2f2 I removed some messages that would be printed if the selector failed because :message is no longer a supported option. What do you think about just letting the default Capybara ElementNotFound error raise? Not enough info?
  • In cd5af47 I updated some of the mercury selectors to be more specific. I needed to do this to prevent capybara from raising an error when it found more than one similar element. Just wanted you to take a glance and make sure those selector seemed reasonable.
  • In 48d7374 I tried to move all the teaspoon configuration over, but there are no javascript specs being found. It seems like the matchers are right, but could you double check the configuration and see if you can see anything. Getting 0 examples, 0 failures with rake teaspoon.

Thanks!

@jedschneider
Copy link
Member Author

there should be 64 passing JS specs, for reference.

expect(page).to have_css('meta[name="keywords"]')
expect(page).to have_css('meta[content="some_keyword, someother_keyword"]')
expect(page).to have_css('meta[name="description"]')
expect(page).to have_css('meta[content="This is a description of the page"]')

Choose a reason for hiding this comment

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

Line is too long. [81/80]

@jedschneider
Copy link
Member Author

@gvarela the newer rspec mock and stub api is a bit more restrictive so I had to remove some expectations that under the old syntax were just not being called. please take a look at da67513 and see if there are any places I removed something that for some reason is of value.

@jedschneider
Copy link
Member Author

@gvarela also like your opinion on the Gemfile.lock file. it is currently not tracked in git. Not sure if this is a desired thing in a gem or not.


it 'removes trailing slashes' do
expect(request).to receive(:params).and_return(path: '//path/').at_least(:once)
expect(Cmsimple::Path.from_request(request).destination.uri).to eq('/some-other-path')
Copy link
Member

Choose a reason for hiding this comment

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

Indentation

@gvarela
Copy link
Member

gvarela commented Mar 12, 2015

This looks good.

Yeah you never check in a Gemfile.lock for a gem. Only for an application.

@@ -26,7 +26,7 @@ def update_content
end

def editor
render :nothing => true, :layout => 'editor'
render :text => '', :layout => 'editor'

Choose a reason for hiding this comment

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

Use the new Ruby 1.9 hash syntax.
Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

@jedschneider
Copy link
Member Author

@gvarela could I get your eyes on these last couple commits, it seems like in rails 4.2 render nothing: true acted differently for us, where it just didn't render the layout at all so, now just calling render with an empty string. have a better suggestion? Then you guys have some overrides for responders to insert messages and handle updates appropriately for mercury. the new responders gem doesn't take a parameter for an internal method called api_behavior and it doesn't have the resourceful? instance method, I don't think. Thoughts on this approach? I'm thinking its not ideal. But out of ideas at the moment.

@jedschneider
Copy link
Member Author

@jejacks0n we're on 0.9.1 for teaspoon.

@jejacks0n
Copy link
Member

@jedschneider cool. Can you just call head :ok if you want to render nothing?

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.

4 participants