Use dom instead of string concatenation like a boss #160

Merged
merged 4 commits into from Aug 11, 2012

Projects

None yet

2 participants

@mstahl

These weren't working on an app that @Bantik and I are working on, so I changed the methods that build inline forms so that they use the jQuery DOM methods instead of concatenating strings together. The concatenation was breaking because we have a select box that has heights in feet/inches in it, which were escaped in our code but when concatenated and dumped to the browser resulted in invalid HTML attributes being created. Using the DOM should be a lot more robust and easier to maintain.

Also added a "height" attribute to the test app's user model with a select box similar to the use case in our app, with tests to accompany it.

mstahl and others added some commits Jun 20, 2012
@rogercampos
Collaborator

I think it's a good idea, but some tests are failling, can you take a look please?

Failures:

  1) BestInPlace::BestInPlaceHelpers#best_in_place with a select attribute should show the current country
     Failure/Error: @span.text.should == "Italy"
       expected: "Italy"
            got: "" (using ==)
     # ./spec/helpers/best_in_place_spec.rb:317:in `block (4 levels) in <top (required)>'

  2) JS behaviour should be able to use bip_select to change a select field
     Failure/Error: page.should have_content("Italy")
       expected there to be content "Italy" in "-"
     # ./spec/integration/js_spec.rb:128:in `block (3 levels) in <top (required)>'
     # ./spec/integration/js_spec.rb:127:in `block (2 levels) in <top (required)>'

Finished in 46.32 seconds
99 examples, 2 failures

Failed examples:

rspec ./spec/helpers/best_in_place_spec.rb:316 # BestInPlace::BestInPlaceHelpers#best_in_place with a select attribute should show the current country
rspec ./spec/integration/js_spec.rb:124 # JS behaviour should be able to use bip_select to change a select field
@mstahl
@rogercampos rogercampos merged commit 7acefe2 into bernat:master Aug 11, 2012
@rogercampos
Collaborator

I solved a little bug in the select and it's merged now, thanks for the work!

@mstahl

No worries! Sorry I kind of got too busy with my dayjob for a while and lost track of this guy. I'll try moving our stuff back over to the mainline version of it and see if it works. (We switched our gemfile to point to my quick fix version for a while.)

@rogercampos
Collaborator

Nice! let me know how it's going for you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment