9Gag now with Meme Title ^^ #1276

Closed
wants to merge 1 commit into from

3 participants

@Shelvak

No description provided.

@technicalpickles
GitHub member

In general, I'd recommend against making changes like single quotes to double quotes while making other changes. It's tempting, but makes reviewing the diffs a lot harder.

@technicalpickles technicalpickles commented on an outdated diff Jan 20, 2014
src/scripts/9gag.coffee
img_src = "http:#{img_src}"
+ response_handler img_title
@technicalpickles
GitHub member

Instead of calling response_handler twice, it might make more sense to update the handler to take another argument, ie (url, title). That way, in the send_meme call above, you can msg.send url, title, which ensures that they get sent in a consistent order. Otherwise, you'll randomly get the title or the url first, which can get pretty annoying to users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Shelvak Shelvak Retouched 9Gag
- Animated Image
- Gag-Title
- Escaped title characters
73f05d8
@dedeibel

It seems to me like the loop to replace the html chars must change like this:

 for r in replacements
-  text.replace r[0], r[1]
 for r in replacements
+   text = text.replace r[0], r[1]
+ return text
@Shelvak

Well dedeibel thanks ^^ I'll probe that, and i'll commit again =)

@technicalpickles technicalpickles commented on the diff Feb 6, 2014
src/scripts/9gag.coffee
@@ -37,16 +37,34 @@ send_meme = (message, location, response_handler)->
location = response.headers['location']
return send_meme( message, location, response_handler )
- img_src = get_meme_image( body, ".badge-item-img" )
+ img_src = get_meme_image( body, ".badge-item-animated-img" )
+ img_title = escape_html_characters( get_meme_title( body, ".badge-item-title" ))
@technicalpickles
GitHub member

You could probably use javascript's built in escape rather than implementing and using escape_html_characters.

@Shelvak
Shelvak added a note Feb 7, 2014

The target of the method is the revert way (in the commit that is wrong) because in basecamp we see the "'" and it's very annoying for us ^^

So well you can close that if want, no problem ^^

A hug

@technicalpickles
GitHub member

I'm not sure I follow 😓

@Shelvak
Shelvak added a note Feb 7, 2014

Sorry github put the correct character =P

I rectify

The target of the method is the revert way (in the commit that is wrong) because in basecamp we see the "& # 0 3 9 ;" and it's very annoying for us ^^
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@technicalpickles
GitHub member

I'm happy to merge now, but it looks like a merge conflict because of the gif addition in the other PR. Can you fix up, and then I can merge?

@dedeibel dedeibel referenced this pull request Feb 10, 2014
Merged

9gag with titles #1319

@dedeibel

Hi,

I already integrated it in another branch and made a pull request: #1319. Maybe you could pull this one. But the credit surely goes to Néstor Coppi.

@technicalpickles
GitHub member

Merged #1319, thanks!

@Shelvak

Well ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment