search

mozilla / universal-search

Completed Test Pilot experiment: add-on that inserts server recommendations into the Awesome Bar
https://wiki.mozilla.org/Test_Pilot/Universal_Search
Mozilla Public License 2.0
29 stars 16 forks source link

Reinstates movie cards. #316

Closed chuckharmston closed 8 years ago

chuckharmston commented 8 years ago

screen shot 2016-09-26 at 5 27 11 pm

This reinstates the movie cards commit without A/B testing, with two required fixes and A/B testing.

r? @6a68

jaredhirsch commented 8 years ago

@chuckharmston Looking at this PR, it appears that the commit 957c823f adds 442 lines and removes 216. This makes it a nearly exact, but not exact, copy-paste of the original commit from pull request #261, which added 440 lines, and removed 211. Could you either revert the original commit and add a separate commit to change those 5-10 lines, or document in this PR which lines were changed?

I don't understand the variant code at a glance, I'll test it out and add review comments tomorrow morning.

chuckharmston commented 8 years ago

As I noted on IRC, 957c823f is a revert commit with a few conflicts fixed in lib/universal-search.js.

jaredhirsch commented 8 years ago

@chuckharmston Cool. And what was wrong with the other A/B PR? Was it not working?

chuckharmston commented 8 years ago

@6a68 It wasn't, as I noted here. At the very least the order of the arguments here is wrong, but correcting that still didn't fix it. I'm not sure how you ever had it working at all.

Since you weren't available for questions, I started mostly from scratch, using code I wrote for one of the example add-ons as a base.

jaredhirsch commented 8 years ago

Cool. Too bad the old PR wasn't working. Looking now

jaredhirsch commented 8 years ago

@chuckharmston Nice work! I do see movie cards when I correct that variants.useMovieCard line, and manually set universalSearch.variants.movieCard = true.

Edited: after pulling in that latest recommendation server commit, I'm now seeing images. They are arriving rather late, but hopefully the image proxy will help when this actually hits production.

Feel free to land this one the variants check is fixed.

search-movie-cards-2

jaredhirsch commented 8 years ago

@chuckharmston Since you're on a flight, and we want QA to look at this as soon as possible, I went ahead and changed that one line in ui/recommendation.js from variants.useMovieCard to variants.movieCard, and force-pushed to your branch. Merging.