{"action":"created","issue":{"url":"https://api.github.com/repos/mozilla/firefox-ios/issues/2106","repository_url":"https://api.github.com/repos/mozilla/firefox-ios","labels_url":"https://api.github.com/repos/mozilla/firefox-ios/issues/2106/labels{/name}","comments_url":"https://api.github.com/repos/mozilla/firefox-ios/issues/2106/comments","events_url":"https://api.github.com/repos/mozilla/firefox-ios/issues/2106/events","html_url":"https://github.com/mozilla/firefox-ios/pull/2106","id":178350844,"number":2106,"title":"Bug 1243412 - Open Top Sites in new tabs using contextual press","user":{"login":"mauryat","id":9949800,"avatar_url":"https://avatars.githubusercontent.com/u/9949800?v=3","gravatar_id":"","url":"https://api.github.com/users/mauryat","html_url":"https://github.com/mauryat","followers_url":"https://api.github.com/users/mauryat/followers","following_url":"https://api.github.com/users/mauryat/following{/other_user}","gists_url":"https://api.github.com/users/mauryat/gists{/gist_id}","starred_url":"https://api.github.com/users/mauryat/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mauryat/subscriptions","organizations_url":"https://api.github.com/users/mauryat/orgs","repos_url":"https://api.github.com/users/mauryat/repos","events_url":"https://api.github.com/users/mauryat/events{/privacy}","received_events_url":"https://api.github.com/users/mauryat/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2016-09-21T13:43:38Z","updated_at":"2016-09-21T19:51:13Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/mozilla/firefox-ios/pulls/2106","html_url":"https://github.com/mozilla/firefox-ios/pull/2106","diff_url":"https://github.com/mozilla/firefox-ios/pull/2106.diff","patch_url":"https://github.com/mozilla/firefox-ios/pull/2106.patch"},"body":"@farhanpatel, could you give me some feedback? :) \r\n\r\nI have a few concerns specifically. Not sure if the below approaches are elegant enough:\r\n\r\n1. To get a reference to `TabManager`'s instance, I had to create properties in multiple classes (`HomePanelVC`, `HomePanels`, `ActivityStreamPanel`, `ActivityStreamTopSitesCell`).\r\n2. To avoid having an optional `TabManager` property, I created initializers in the respective classes."},"comment":{"url":"https://api.github.com/repos/mozilla/firefox-ios/issues/comments/248723199","html_url":"https://github.com/mozilla/firefox-ios/pull/2106#issuecomment-248723199","issue_url":"https://api.github.com/repos/mozilla/firefox-ios/issues/2106","id":248723199,"user":{"login":"farhanpatel","id":522281,"avatar_url":"https://avatars.githubusercontent.com/u/522281?v=3","gravatar_id":"","url":"https://api.github.com/users/farhanpatel","html_url":"https://github.com/farhanpatel","followers_url":"https://api.github.com/users/farhanpatel/followers","following_url":"https://api.github.com/users/farhanpatel/following{/other_user}","gists_url":"https://api.github.com/users/farhanpatel/gists{/gist_id}","starred_url":"https://api.github.com/users/farhanpatel/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/farhanpatel/subscriptions","organizations_url":"https://api.github.com/users/farhanpatel/orgs","repos_url":"https://api.github.com/users/farhanpatel/repos","events_url":"https://api.github.com/users/farhanpatel/events{/privacy}","received_events_url":"https://api.github.com/users/farhanpatel/received_events","type":"User","site_admin":false},"created_at":"2016-09-21T19:51:12Z","updated_at":"2016-09-21T19:51:12Z","body":"Hi! Thanks for this PR. \r\n\r\n@bkmunar is working on a context [menu](https://bugzilla.mozilla.org/show_bug.cgi?id=1297128) that will build out this menu for the recent items below the topsites. The work you did here would be great because he isn't building out the ability to open in a new tab yet. Once he finishes the menu you can use that instead. You can follow the progress of that in the bug I linked above. \r\n\r\nBut I'd suggest design wise that you try to do the following. \r\n- Try to move all changes in the TopSiteCell into ASPanel. For example I have the `self.topSitesManager.presentActionMenuHandler`. You could rework this so that all the menu creation and presentation happens in ASPanel instead. We can reuse the logic @bkmunar is making that way!\r\n- Look at how the tabManager is used in the MenuViewController. That might give you ideas on how to structure tabManager. (That would be a good place to look. I'm not sure if the menu is doing anything different though!)\r\n\r\n![newmenu](https://cloud.githubusercontent.com/assets/522281/18725982/4b1bb89a-7ff7-11e6-809c-604ee813fb15.png)\r\n\r\nYou can still reuse a lot of this work once @bkmunar merges his new context menu for the recent items. We'll need to get the new menu working with the TopSites portion too. \r\n\r\nI'm sorry we had not closed this bug earlier. This is one of the select bugs that has become outdated by the new ActivityStreamPanel. If you want to look at work related to the ActivityStreamPanel you can follow it here https://bugzilla.mozilla.org/show_bug.cgi?id=1288121\r\n\r\n"}}
{"actor":{"display_login":"farhanpatel"}}