search

spree / deface

Rails plugin that allows you to customize ERB views in a Rails application without editing the underlying view.
MIT License
521 stars 128 forks source link

Wrapping around contents of whole template #23

Open angelim opened 12 years ago

angelim commented 12 years ago

I've been trying to wrap the contents of a partial, irrespective of its markup, by selecting the first and last elements. Although the :surround action doesn't support using a range of elements with:closing_selector I was under the impression that I could replicate the desired behavior using a combination of overrides. Unfortunately I couldn't even select the first document element with Nokogiri selectors. The *:first syntax returns the first nodes for each element type. This is not a Deface bug, but I couldn't find a group/list to post this sort of question. Thanks

maximkulkin commented 12 years ago

I agree. My attempt to solve similar problem (append to the end of the partial) was to use ":root:last-child" selector. This seem to be broken in Nokogiri implementation. I believe, the roots are in the broken implementation of DocumentFragment class which uses magic of wrapping contents with elements and reassigning parents for some nodes. I believe, in order to make it work correct, we need to rewrite code to always use Document class instead of DocumentFragment and do wrapping manually. Then, the documentation should clarify that there is always and tags and users should account for that if they want to select root elements (e.g. "body > *:last-child").

angelim commented 12 years ago

Thank you @maximkulkin. I ended up replacing the whole template instead of using Deface on account if this particular problem. Are you suggesting that this override should/could be made in Deface or are you planning on posting an issue in Nokogiri? If we can't do anything here, at Deface's side, I'll close this issue.

maximkulkin commented 12 years ago

I don't want to replace the whole template. In my case I have a partial that lists items for menu and I just want to add just another menu item.

I posted an issue to Nokogiri, but this situation is kind of non-standard (XML/HTML shouldn't have more than one root nodes) and I would't expect them to fix this in any way.

I think that Deface should have means to deal with such situations. Maybe it could be just a special case when the selector equals "document" (":document", ":partial" or ":template"), then treat it specially allowing to insert to top/bottom/both.

angelim commented 12 years ago

Let's hope someone from the core team pick this up. I think this implementation is beyond my skill level.

If you're trying to append an item to an ul, can't you get a handle on its selector? Maybe the list definition is in another file and you iterate on the child partial and you're trying to append there(that would make more sense to explain your problem). If that's the case I think you could go up one level and append items in the list definition file.

In my case, I wanted to wrap a series of files with a div of a particular class. Unfortunately I wasn't able to do so and replaced several files in my project, which will certainly be a big problem in future spreecommerce updates.

maximkulkin commented 12 years ago

Yes. You're right about my problem. For now I've solved it with targeting particular menu item (which happens to be last currently):

:insert_after => 'code[erb-loud]:contains("...")'

Moving this to upper level could be a legit move unless this partial is used more than once, which will require patching all places.

BDQ commented 12 years ago

@maximkulkin I don't really understand your first comment regarding the use of DocumentFragment vs Document.

If I use Document it will wrap the elements with:

<html>, <body> tags automatically - which is a problem for most partials.

I think your idea of a special selector is a really good idea, so you still get to use the actions. I'll look into that as soon as I get a chance.

@angelim - both :surround and :surround_contents now support :closing_selector (only on the dsl branch). But it still won't solve your problem by the sound of it.

maximkulkin commented 12 years ago

@BDQ My comment on DocumentFragment was there because Deface::Parser uses it if document source doesn't have "html" or "body" tags. In this case DocumentFragment class is used (see lib/deface/parser.rb:99).

BDQ commented 12 years ago

Ohh and there's an issue with certain selectors processed against DocumentFragement which work against Document?

maximkulkin commented 12 years ago

Yes. For example this.

This issue can be worked around by using Document class instead of DocumentFragment and wrapping multiple root elements manually. The other solution would be to wait for Nokogiri fixing this, but DocumentFragment behavior is not specified by standard anyways and using only standard stuff would be better.