Added #link into text.rb + tests

[Pablo-Merino]

Added a #link method to text.rb since @sandal said in #689 (last comment) that a method was needed. Added tests too. Since this is my first PR in this repo, I'd love if any of you could tell me if I did anything wrong, so I can fix it. Thanks 😃

[pointlessone]

Early return, suffix conditional, flow control or in conditional, all in one line? 😭
How about this?

if title && url
  # ...
end

[Pablo-Merino]

Hey, thanks for the feedback. I've thought about using what other functions were using. Just realized this is not a good way. My bad! Will commit a fix now

[pointlessone]

Do you think it should return anything? This is sort of fire and forget method. Also by default it would return nil which is falsey, too.

[Pablo-Merino]

Well, the other methods for adding text return false when the text is nil, so I figured this one should too.

[practicingruby]

I don't know how well we thought out this interface, but if right now we're returning false to signal non-rendering, we might as well be consistent. So it's OK to leave as-is.

[Pablo-Merino]

Alright, @sandal. Just reverted it with your suggestion 😃. Feel free to merge it!

[practicingruby]

@Pablo-Merino: Overall this looks great, and it's a nice feature addition.

I think I will want a more substantial change before merging, though it's not a complicated one:
Rather than using text with inline format, I'd prefer to use formatted_text, which takes an array of hashes of text fragments.

The reason I'd prefer to do it this way is it pushes us one level deeper down the stack, bypassing some levels of complexity and making it more likely that this feature won't break if we ever make major changes to the top-level text method.

formatted_text is documented in the manual so if you want to switch it up to use it, go for it. Otherwise, I'll make the change when I'm ready to test and merge this code.

[Pablo-Merino]

@sandal: sure, let me make the change and I'll commit :)

[practicingruby]

Almost there, but not quite. The idea is to avoid unnecessary use of HTML tags, because then we bypass that subsystem. Calling text_formatter.format() is roughly equivalent to calling text() directly, because it's going to walk the same code path.

What I'm suggesting is something like this:

formatted_text([{ :text => link_name, :link => link_url }], options) 

You can update the request if you'd like, but it's a trivial change so I don't mind making it when I pull down the code to test it.

[Pablo-Merino]

Oh, haha I was in a hurry and didn't properly read the docs. I won't be home until a few hours from now, but I can do this change then, but I don't mind if you do it now.

El 8/9/2014, a las 18:52, Gregory Brown notifications@github.com escribió:

Almost there, but not quite. The idea is to avoid unnecessary use of HTML tags, because then we bypass that subsystem. Calling text_formatter.format() is roughly equivalent to calling text() directly, because it's going to walk the same code path.

What I'm suggesting is something like this:

formatted_text([{ :text => link_name, :link => link_url }], options)
You can update the request if you'd like, but it's a trivial change so I don't mind making it when I pull down the code to test it.

—
Reply to this email directly or view it on GitHub.

[Pablo-Merino]

@sandal My bad for not doing it as you said. The latest commit should be good to go now

[practicingruby]

This looks good now! We should probably add a manual entry for it, though I'm not sure exactly where the right place is for it, so I'll figure that out when I merge.

Thanks a lot @Pablo-Merino, I'll get this into master in time for the 1.3.0 release, which will hopefully happen soonish.

[Pablo-Merino]

@sandal alright, that's good. You're welcome! 😃

[practicingruby]

I just went back and looked at my comment on #689, and realized it's not really related to the feature on this pull request. What I was talking about in #689 is how it's very difficult to link arbitrary content in the document (such as an image) because it requires using a very low-level interface.

So the higher level interface I had envisioned for that would have been something like the following method, which would generate a clickable region on the document.

link "http://example.com", :at => [x,y], :width => w, :height => h

But the feature implemented on this pull request is just about making standalone text-based links. I'm not really sure how common of a use case that is, because most text links I can think of are embedded inline in paragraphs. For that purpose, we have both the inline styling <link> tag, and the {:text => "Link Name", :link => "http://example.com"} option for use with formatted_text.

Putting all this together, I'm worried this patch will take up a name we're going to want to use later for a more generic purpose, and that it'll only serve as a convenience method for a small percentage of the use cases for text links.

I should have given this more thought earlier, but the good documentation, tests, and overall friendliness of @Pablo-Merino's pull request made me feel like this was a good addition without considering these deeper issues.

On closer review I don't think this should be merged. I should have mentioned that much earlier, but I only really thought about these problems now.

Sorry for dropping the ball on this one!

[practicingruby]

@Pablo-Merino: Even though we probably won't merge this patch, I've given you commit access to Prawn. You really were amazingly responsive to our requests for revisions on this patch, and we'd absolutely welcome your future contributions. For guidelines, see here:

https://github.com/prawnpdf/prawn/wiki/Contributor-welcome-notes

[Pablo-Merino]

Alright my bad! I didn't really specify what is this for.

I'll try to get your idea working as soon as I can, I don't think it should be too hard.

El 10/9/2014, a las 15:24, Gregory Brown notifications@github.com escribió:

Closed #764.

—
Reply to this email directly or view it on GitHub.

[practicingruby]

@Pablo-Merino: A patch for #689 would be awesome! I agree, it shouldn't be complicated, and if you implement it we'd be helping folks who want to have linkable images.