Spice plugin to display facebook fanpage info #136

Closed
wants to merge 3 commits into
from

Projects

None yet

4 participants

@sidchilling

Displays the information of facebook fan page. The search query can be something like -

" facebook cafepress "

So this will read the "facebook" string and will comprehend the next word as the fan page and retrieve the info from facebook.

@yegg
DuckDuckGo member

Hi! We've needed facebook integration so thanks for starting this thread. However, I'm confused why there are @hunterlang and @nospampleasemam commits in this pull request?

@sidchilling

@yegg Yes, I don't understand that. This is my first ever pull request on github... have I done anything wrong here that @hunterlang and @nospampleasemam are also included in the pull request?

@yegg
DuckDuckGo member

@sidchilling possibly though not sure what yet :). @hunterlang and @nospampleasemam were moving over all the documentation to github and it appears it is somehow in your pull request. They should know better than me what they were doing so I hope they can chime in and specify/fix.

@majuscule
DuckDuckGo member

This is strange. The only explanation I can think of is that someone force pushed to master, who hadn't fetched hunter and my commits. :-/ That really shouldn't happen.

I've looked over the new code, and it looks good! We actually tried writing a facebook spice once before, but had problems with their search mechanism. I think fanpages is a great way to make this work, and maybe we can figure out how to expand it from there as well.

It's live on dylan.duckduckgo.com right now. I do have a few suggestions for changes, such as the image layout. Generally, all of our zeroclickinfo with images place them in the upper right corner. There's actually even an 'i' attribute for an image on the items object. :-) The Gravatar spice is a good example of that functionality.

I'm going to play with it a bit to see what else I can recommend, and I'll post back here.

It would also be appreciated if you add a test file to the t/ directory. If you take a look inside, there should be one for each other currently existing spice. They're pretty straightforward, and help us make sure that plugins will continue to work after future changes. There's also documentation for them at the very end of duckduckhack.com (under "Advanced Testing"). If you have any questions, please don't hesitate to ask!

Thanks,
Dylan

@majuscule
DuckDuckGo member

Just to clarify, @sidchilling , you didn't do anything wrong! In fact, you saved us from eventually having to rewrite those two commits. I am going to cherry-pick them into our mainline now. Thanks again!

@majuscule
DuckDuckGo member

Hi again,

@moollaza pointed out that it looks like you wrote your spice.js in coffeescript and compiled down to JS. There's nothing wrong with that exactly, but the JS it generates is unwieldy. Because we want all of our open source code to be as friendly for newcomers as possible, it's best to write the javascript natively. I've read over the code though, and it's fairly straightfoward. It shouldn't be too bad to rewrite, or refactor using the current code as a starting point.

Another thing that will help, is putting the CSS into a spice.css file, next to the spice.js file in share/facebook_fanpages/. Separating the style rules like this also helps with clarity and maintanibility.

One last note, I think you've made a typo when defining the header. items[1]['h] should be items[0]['h'].
// sorry, that's not in your code, my mistake, I'd delete this but didn't want to cause confusion.

Let me know your thoughts :-)

Thanks!

@yegg
DuckDuckGo member

@nospampleasemam still concerned about how these commits got in there. Can you trace down the force push? I'm wondering if any new commits are additionally missing.

@majuscule
DuckDuckGo member

I'll see what I can figure out, but generally, due to the nature of a force-push, it's not possible to find the modification point. It was probably noticed back then by myself (and anyone else working with spice), but written off as a local change lost track off and unneeded.

I think it would be a good idea to warn everyone to never force-push to master. It's okay on a feature branch where you are likely working alone.

@yegg
DuckDuckGo member

@nospampleasemam yes please send out an email :). Also, can we disable it somehow like we can internally?

@majuscule
DuckDuckGo member

Blocking force-pushes needs to be done inside a pre-receive hook inside the hosted repository. GitHub (non-enterprise) doesn't support this right now, despite what looks like an old (and now deleted) feature-request. I've sent them an email, referencing what I found and asking if there's any update.

Sending an email now, and I'll make a task for us to talk about this more, I don't want to keep filling up @sidchilling's pull request :-)

@sidchilling

Hi @nospampleasemam Thanks for the suggestions. I think the following could be done with no problem -

  1. Using i option in items for the image of the page.
  2. Writing the spice.js in native JS. However, I do think that does not make a difference. If a coffee file is available, then it is that file that should be read and not the js file.
  3. Moving styling to spice.css

Also, I am thinking about placing a facebook like button in the HTML content (if that is possible), so that people can Like from the search result - your thoughts on this?

Also, I have a question - should I continue work on my local forked repository and push new code there itself and it will automatically be linked to the pull request? Or is there something else I need to take care of?

@majuscule
DuckDuckGo member

You can continue to work on your local forked copy of the repository and your commits will automatically be added to this pull request, as long as you work on the master branch, because this pull requests points at your HEAD, rather than a specific commit.

I understand what you're saying about coffee, but we do not support it at this time, and likely will not ever. We are however, working on "Spice 2", which will offer a new way to write spice that will hopefully be a bit more comfortable for you. That won't be live for a while yet though.

The facebook like thing is a good question. @yegg do you have any thoughts on adding a facebook like button? I think at first blush, it's a good idea, (since "facebook" is in the query). But at the same time I can't help but recoil a bit worrying about what kind of tracking they do with those images/scripts. Can we do this safely?

Thanks! I'm excited to see the changes.

@yegg
DuckDuckGo member

No, we cannot allow any third-party javascript (that is called on the fly) for privacy reasons.

@majuscule
DuckDuckGo member

Hi again @sidchilling,

We've released our new Spice platform, which uses a new render and templating system. It was written to make it even easier for contributors to develop Spice plugins. I'm closing this pull request for now - but please check out our new documentation, let us know anything that can be improved, and feel free to reopen this pull request. We'd love to have it!

Thanks again,
dylan

@majuscule majuscule closed this Jun 25, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment