Sounds good. Ill submit the patch shortly
On Fri, Sep 13, 2013 at 12:16 PM, Erin Noe-Payne <[email protected]>wrote: > Agreed with Dan. > > On Fri, Sep 13, 2013 at 6:59 AM, Dan Gornstein <[email protected]> wrote: > > On Thu, Sep 12, 2013 at 7:53 PM, Rohit Kalkur <[email protected] > >wrote: > > > >> Hey everyone, > >> > >> I am writing a check for the ng-hide directive of the "Add widgets to > this > >> page" message (which is displayed on any Page that does not have any > >> widgets on it). > >> > >> For now, I have added a method to the PageResource object called > >> hasRegionWidgets(), which basically determines whether any of the > regions > >> for that page have region widgets. > >> > >> However, I was examining the tabs.html and thought eventually it might > make > >> sense to refactor the CurrentPageCtrl into 2 separate controllers: one > for > >> handling the current page tab functionality (edit page, move page, > delete > >> page, etc...) and one for managing operations on the current page itself > >> (sending the current page info to the widget store). > >> > > > > +1 I think this makes sense. We don't want the controllers to get too big > > and handle too much so that they can be reused by people for specific > > purposes. These seem like two different parts of functionality and I > > believe make sense to be split up. > > > >> > >> If we go with that design, it might make sense to move the > >> 'hasRegionWidgets check' to the controller that would manage the current > >> page operations. > >> > > > > +1 > > > >> > >> Anyhow, I just wanted to poll the dev list to see what everyone's > thoughts > >> were. For now, I was thinking of just submitting the patch with the > >> hasRegionWidgets() method on the PageResource and then later on down the > >> road if we go with the 2 split controllers, we can just refactor > >> everything. > >> > > > > I think submitting the patch for now is fine as we are in active > > development and it is easy to change once some more people weigh in on > the > > options or conform to lazy consensus. > > > >> > >> Let me know what you think. > >> > >> Thanks! > >> -Rohit K > >> >
