Patch (to be ported)
Project:
Facebook-style Statuses (Microblog)
Version:
6.x-3.x-dev
Component:
User interface
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
2 Jul 2011 at 19:58 UTC
Updated:
8 Feb 2013 at 09:39 UTC
Jump to comment: Most recent file
Comments
Comment #1
icecreamyou commentedThanks for the patch -- unfortunately the only reason why it works is because browsers are pretty good at inferring the expected behavior of broken HTML. I've committed it to FBSS 6.x and Statuses 7.x, but a more comprehensive solution is needed. Specifically, we need to make sure that we don't end up with overlapping HTML tags, like this:
This is a difficult problem and not one for which I am able to write a solution without learning something new.
Comment #2
rabisg commentedWhile trying to patch this, came across a additional problem.
Instead of truncating the text to the size mentioned in Configuration/Statuses/Advanced it truncates the HTML text to that size.
Attaching patch for that too.
This should fix it too.
Comment #3
icecreamyou commentedNot sure why you made this change but it will break if there is no input filter being used or there are multiple paragraphs in a status or there is other markup like tables. The truncating should run on the entire status, not per-paragraph.
Couple of things:
Looking back on it now I am not sure why the "readmore" stuff is even in there, it doesn't seem to be used for anything. Probably cruft. So it's fine to remove that.
Comment #4
rabisg commentedSorry my bad, forgot to remove that before creating the patch.
Also I can write the desired function(truncate HTML) on my own. An easy way would be to just pull out a substring, append readmore link and expect Jquery to close the unclosed tags(which it does gracefully). But since I read it somewhere that reusing code is a good idea, I decided to use it.
Thanks for the tip on licenses. I'll take care of it next time. Do you want me to submit a modified patch?
Comment #5
icecreamyou commentedYes, what I'd like to see is a patch that uses Drupal's built-in text_summary() to do this instead of writing a new function or using someone else's, as suggested in #3 above. You're right that generally reusing code is a good idea, but better to reuse code that we already have. :-)
Comment #6
rabisg commentedModified _statuses_run_filter() and statuses.js
Comment #7
icecreamyou commentedthis line can be removed too I think
trailing spaces
trailing space; also the default value of this variable is 'none' not NULL so it needs the 'if' statement check
Indented too far... just indent 2 spaces from $text. Also since we're doing this in PHP instead of JS now, let's use l() to create the link instead of manually typing out
<a href="#" class="....These are very minor issues, overall the patch looks good. Just fix these syntax things and it's RTBC.
Comment #8
mathankumarc commentedCan we go ahead and commit this patch and close the issue?
Comment #9
icecreamyou commentedIf the issues from #7 are fixed this is RTBC from me. It will also need a backport although I believe the changes should be identical.
Comment #10
icecreamyou commentedUpdated but untested patch attached. Should be ready, but will also need a backport.
Comment #11
icecreamyou commentedCommitted #10 to dev with a slight change, after testing.
Comment #12
icecreamyou commentedWhoops, forgot this needs a backport.
Comment #13
mathankumarc commentedCan we move this issue back to FBSS issue queue?
Comment #14
icecreamyou commented