Enhance position option with :append and :prepend options to inject code at start or end of head or body tags

[salimhb]

No description provided.

[DonSchado]

hm... I mean it's not critical but I don't really like the semantics here. With your change position is not precise anymore. Maybe we should call it container or something, and let the position be opening/closing ?

[salimhb]

Yeah. I wasn't sure about the semantics here either. But I wanted to just get this PR out to get feedback about this.
:container/:position actually sound better. Thanks :)

[salimhb]

done now with container_tag and container_position. I avoided changing the position attribute at first to avoid changing it everywhere.

[jhilden]

I find the the wording with :opening and :closing not 100% ideal. What about using jquery as a well known inspiration and using :prepend and :append instead?

[DonSchado]

👍 for prepend/append

[salimhb]

That also sounds good. Getting more inspired by jquery, I was also thinking of using inside to specify the head or body tag. So it would be:

self.position_inside = :head # or :body
self.position_insert = :append # or :prepend

[DonSchado]

I mean, yes it's a first draft. But what I totally dislike about this code is the responsibility of the tracker.
It was not optimal before, but now it's even worse. Why do the tracker need to ask the handler about his position, to again decide which "message" to send to it again.... weird. The handler should take care of all that.

[salimhb]

this inject method gets the response, changes it, and returns it.
If we want to move this inject into the handlers, then we would need to pass the response again to an inject method inside each handler. Doesn't that make it slower and more difficult to understand what is happening? It would also allow the handler to change whatever else it likes in the response!

Another way I could think of doing this is to group handlers according to their container_tag and container_position, render them together, and then gsub once for each group. What do you think about that?

[DonSchado]

I'm with you on the response.gsub part,... but I think the behavior what comes first/last (the position) should be inside of the handler.

btw. </#{handler.container_tag}> doesn't match elements with attributes right?

[salimhb]

Then what do you think about the grouping idea?

And yeah, good catch, that affects the code 2 lines below! I'll change it and add a spec for that.

[salimhb]

a17b6c0 should take care of the element with attributes issue

[DonSchado]

hm position_inside/position_insert sounds weird too imho.

why not give it a method and let it look more like a DSL?

position body: :prepend

[salimhb]

I discarded the idea of using a hash at the beginning, but this way it looks actually nice.

[kangguru]

i like @DonSchado idea, we might even go further and get rid of posistion and just have

prepend: "<this is my tag>"

or

append: "<this is the tag>"

also there are already some issues with the naive regex approach, we should not drive further into this direction and opening new potential problems

[DonSchado]

salim pls also consider to address this issue here: #28

gsub is bad! ;)

[salimhb]

#28 can be resolved if we first render the handlers for a certain position together, then inject them in one step.
Any suggestions other than gsub? we could use Nokogiri, but that messes a lot with the response.

[DonSchado]

in which state is this now? do you need any help on this? or feedback on a specific problem?

[salimhb]

This is done with the following format now:

self.position body: :append

or

self.position body: :prepend

Of course, it has to be updated with the latest master.

[DonSchado]

first of all, I'm cool with the use case, but to be honest I'm not fully satisfied with this solution.
let's grab some coffee and talk about it later? :)

[salimhb]

let's do that 💚

[DonSchado]

😴

;)

[jhilden]

I would like to fix this up, so it can be merged.

What exactly would need to be done differently?

Are we OK with the positioning syntax? self.position body: :prepend
(or maybe self.position body: :top?)

[bumi]

closing this PR in favor of #84

That PR allows handers to define a inject method and manipulate the response directly which gives the handlers quite some flexibility. Though it is not possible for the user to do more position configuration for now.
if somebody needs more flexibility please open a new PR based on the latest master.