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

When using :display_with shows text AND nil value. Only allows editing of nil value. #331

Open
betjaminrichards opened this issue Feb 22, 2014 · 20 comments

Comments

@betjaminrichards
Copy link

I have the following code:

<%= best_in_place_if can?(:update, risk), risk, :effect, :display_with => :simple_format, :path => assessment_risk_path(assessment,risk), type: :textarea, html_attrs: { "class" => "input-block-level" }, :nil => "Click to add a effect..." %>

What I get on screen is

Click to add a effect...

Sed ut perspiciatis unde omnis iste natus error sit voluptatem accusantium doloremque laudantium, totam rem aperiam,

eaque ipsa quae ab illo inventore veritatis et quasi architecto beatae vitae dicta sunt explicabo.

Nemo enim ipsam voluptatem quia voluptas sit aspernatur aut odit aut fugit, sed quia consequuntur magni dolores eos qui ratione voluptatem sequi nesciunt. Neque porro quisquam est, qui dolorem ipsum quia dolor sit amet, consectetur, adipisci velit, sed quia non numquam eius modi tempora inci

The attribute's text is correctly formatted, but:

  1. Why is it showing both the nil text AND the attribute's actual text?
  2. I can only click and edit the "Click to add a effect..." text, not the actual attribute's text

Any ideas?

@sebastialonso
Copy link

Which version of Rails are you using?

@betjaminrichards
Copy link
Author

Ruby: 2.0.0p353
Rails: 3.2.14

@osuthorpe
Copy link

I am getting the same error. When I use (display_with: :simple_format) I get both the nil text and the saved attribute

@timcase
Copy link

timcase commented Jul 22, 2014

Changing the best_in_place helper method so that it wraps the markup it generates into a div instead of a span fixed this for me. I used :display_as with a textile field rendered as html, and I noticed that calling jQuery's html() method on a span with a

tag inside of it resulted in nil getting returned, which then got interpreted by BIP to display :nil. The jQuery html() nil is probably due to a

tag inside of a tag being regarded as invalid HTML 5.

@seuros
Copy link
Collaborator

seuros commented Jul 22, 2014

Can you send a PR ?

@seuros
Copy link
Collaborator

seuros commented Jul 22, 2014

@bensaufley , @pjmartorell, @vjt , @the-robear , @sao, @outomyelement : what do you thinkg about changing it to div ?

@bensaufley
Copy link

span and div have different uses – I'd be wary of making that change by default because people may be relying on inline content. I'm having trouble understanding @timcase's explanation because it looks like his comment got malformatted? Make sure to wrap html tags in ``: <p/>, for example. Is that what you were saying, @timcase? Wrapping a `

` in a `` is invalid HTML, yes.

What if we had an option for it? container: :div for example? Defaults to :span?

@seuros
Copy link
Collaborator

seuros commented Jul 22, 2014

👍 for the container: :div.
I think the javascript will be compatible with this change.

We could also make it better by have the gem configurable

#initializers/best_in_place.rb
BestInplace.configure do |config|
   config.container = :div
end

@bensaufley
Copy link

That's a great idea. I can tackle the content_tag override, pretty easy, but I'm not sure where to put the config defaults etc? Haven't done a ton of gem work. This StackOverflow suggests there should be a self.configure somewhere to get your implementation working, and it doesn't look like that exists?

@seuros
Copy link
Collaborator

seuros commented Jul 22, 2014

I can do the configure part. I will do it asap, since your task depend on mine. it will consist mostly in writing some specs.

What worry me about this gem, is the thread safety issue with display_* . I think we have to find another event if that break the api.

@seuros
Copy link
Collaborator

seuros commented Jul 22, 2014

To reproduce the issue

  • visit a page that has a field with display_with, display_as
  • restart your app (in multi server setup, the load balancing can send you to another server)
  • modify the field
  • the display_* is not set.

@bensaufley
Copy link

But that's a known issue, right, even in master?

@seuros
Copy link
Collaborator

seuros commented Jul 22, 2014

Yes, it was here since the introduction of display_as, 2 year ago. I noticed that while reading the code.

@bensaufley
Copy link

Yeah. So, definitely a concern but not something blocking from merging, fixing, updating other things.

@seuros
Copy link
Collaborator

seuros commented Jul 22, 2014

Yep. we could do it in version 3.1

@timcase
Copy link

timcase commented Jul 22, 2014

I tried to use display_as on an attribute in my model that contains Textile formatted html. Textile wraps all of it's content in p tags. P tags inside of a span is invalid HTML, span is an inline element and therefore you can't have block-level elements (such as a p, div, h1, table etc.), inside of a span. When best_in_place uses element.html(), it's receiving a nil because, I suspect, the invalid HTML is not available to the DOM and thus not available to be inspected by jQuery. On lines 42 and 75 of https://github.com/bernat/best_in_place/blob/master/lib/best_in_place/helper.rb, I changed the span to a div, because div's can hold other block-level elements.

I forked the repo and tried running the specs but I got lost at the part on cding into test_app. I couldn't find the test_app dir in the codebase so I have no idea if this breaks anything except to say that so far it's working quite dandy for me and I've already sent the change to my production environment.

@sao
Copy link

sao commented Jul 22, 2014

@bensaufley @seuros I agree on the content param 👍

@seuros
Copy link
Collaborator

seuros commented Jul 22, 2014

I pushed the configure part

BestInPlace.container = :div

You can also use the block syntax when we will add other options.

We need test for the js.

@betjaminrichards
Copy link
Author

Hi, is there any progress on this being fixed? i.e. Is it on the roadmap? The ability to be able to render carriage returns etc in textareas is a pretty common requirement I would imagine.

@stereodenis
Copy link

@seuros display_with (;

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

No branches or pull requests

8 participants