Parametrize break sequence for word_wrap on ActionView Text Helpers #21086
Conversation
|
@bukue could you please add documentation about it? |
|
@meinac done! |
| @@ -366,6 +366,10 @@ def test_word_wrap_does_not_modify_the_options_hash | |||
| assert_equal options, passed_options | |||
| end | |||
|
|
|||
| def test_word_wrap_with_custom_break_sequence | |||
| assert_equal("1234567890\r\n1234567890\r\n1234567890", word_wrap("1234567890 "*3, :line_width => 2, :break_sequence => "\r\n")) | |||
There was a problem hiding this comment.
New hash syntax please. And add spaces around *.
|
@bukue doing great. Now we just need you to squash your commits down to one 😁 |
bukue
force-pushed
the
add_break_sequence_option_to_word_wrap
branch
from
db8cac8
to
2bf9b54
Compare
|
@kaspth done! |
|
I'm a bit dubious as to whether we actually want this... but that notwithstanding, shouldn't the |
|
@matthewd Why are you dubious? The main reason to split with "\n" is to split by lines. I see how it would be useful to split it by "\r\n" or any other sequence that helps defining a line. Unless somebody else considers it not useful, I will gladly implement it. |
|
@matthewd added the changes you suggested |
|
Why would your What does this offer over If I use a "Great!, this is just what I need... I'll wrap on |
|
Yeah, you can totally have the same result doing word_wrap(...).gsub("\n", "\r\n"). Actually, this PR started because we were doing the exact same thing at work because we needed the end of lines to be indicated with "\r\n". I see your point, although I feel this changes give you more legibility and flexibility. Let's see what the other guys think. |
|
This feels like way too much code for me for something that can be done with |
|
I agree with @kaspth. If you are doing this many times in your application then I recommend you to create a method which does what do you want for your application. |
|
This approach is cheaper since you don't have to transverse your string twice like you would do using gsub. Also, all I am doing is parametrizing things that otherwise would be hardcoded. This would be my first commit to the rails repository so I might be going to far with my changes. So if you guys believe this change is too much I am happy to roll it back to just parametrizing the line break. What do you guys think? |
| def word_wrap(text, options = {}) | ||
| line_width = options.fetch(:line_width, 80) | ||
| line_splitter = options.fetch(:line_splitter, "\n") |
There was a problem hiding this comment.
Lets take out the line splitter.
bukue
force-pushed
the
add_break_sequence_option_to_word_wrap
branch
from
4854e44
to
c10ddae
Compare
…d as today the only option we have is "\n" and is hardcoded.
With this change you will be able to specify any break sequence ("\r\n" for example) as an option.
adding proper documentation for break_sequence in ActionView::Helpers::TextHelper.word_wrap
adding some more documentation for word_wrap custom break sequence and making sure we use new hash syntax
bukue
force-pushed
the
add_break_sequence_option_to_word_wrap
branch
from
c10ddae
to
cf93c6a
Compare
|
@kaspth All done! |
…d_wrap Parametrize break sequence for word_wrap on ActionView Text Helpers
|
Thanks |
`word_wrap` was changed to use kwargs in rails#21086, so it cannot modify the options hash.
Sometimes you need a specific break sequence while using word wrap and as today the only option we have is "\n" and is hardcoded.
With this change you will be able to specify any break sequence ("\r\n" for example) as an option.