Hi, Looks good to me.
Regards, Murtuza On Mon, Jul 31, 2017 at 2:59 PM, Ashesh Vashi <ashesh.va...@enterprisedb.com > wrote: > On Mon, Jul 31, 2017 at 2:54 PM, Akshay Joshi < > akshay.jo...@enterprisedb.com> wrote: > >> Hi All >> > >> On Fri, Jul 28, 2017 at 1:51 PM, Dave Page <dp...@pgadmin.org> wrote: >> >>> >>> >>> On Thu, Jul 27, 2017 at 2:41 PM, Akshay Joshi < >>> akshay.jo...@enterprisedb.com> wrote: >>> >>>> Hi All >>>> >>>> As in commit "Update alertify alerts to use the styling defined in >>>> the style guide": >>>> >>>> https://git.postgresql.org/gitweb/?p=pgadmin4.git;a=commitdiff >>>> ;h=2a30a86e7d5e562040500f448fbb0d143ff2cff9 >>>> >>>> https://git.postgresql.org/gitweb/?p=pgadmin4.git;a=commitdiff >>>> ;h=f2d2075d81718ec02550fb592851aa330d327b24 >>>> >>>> We have introduce new wrapper class "AlertifyWrapper" and replace >>>> calls to alertify.success and alertify.error with following two lines >>>> in most of the files >>>> >>>> var alertifyWrapper = new AlertifyWrapper(); >>>> >>>> alertifyWrapper.success(message); or alertifyWrapper.error(message); >>>> >>>> For each call we are creating dynamic object of AlertifyWrapper and >>>> call the appropriate function. For example there are 20 such calls in a >>>> single js file every time are are creating object and call appropriate >>>> function. >>>> >>>> I have tried to improve the logic here and implemented it as below: >>>> >>>> - Extend alertify and move success, error and info functions from " >>>> alertify_wrapper.js" file to "alertify.pgadmin.defaults.js", there >>>> will be no use of "alertify_wrapper.js" >>>> - Modify only "server.js" as POC, remove 'alertify' and replace >>>> 'sources/alerts/alertify_wrapper' with 'pgadmin.alertifyjs' which >>>> is nothing but mapping of "alertify.pgadmin.defaults.js" from >>>> defines and named the reference object to 'alertify' so no need to >>>> change >>>> any function call like "alertify.success, alertify.error". >>>> >>>> One more benefit of the above approach is if in future we want to use >>>> the same style for alertify.warning, alertify.info, alertify.message >>>> etc.., we will just have to extend that method in "alertify.pgadmin >>>> .defaults.js" and no need to change the rest of the function call with >>>> AlertifyWrapper. >>>> >>>> Attached is the POC patch, if it looks good then I'll start working on >>>> replacing AlertifyWrapper with the above mentioned approach. >>>> >>> >>> I like the approach - it's definitely cleaner, and saves instantiating a >>> new object every time. >>> >> >> I have modified the logic to improve the usage of alrtify >> notification. Attached is the patch file which contains following: >> >> - Replace 'alertify' with 'pgadmin.alertifyjs' in define[]. >> - Remove 'sources/alerts/alertify_wrapper' from define[]. >> - Replace calls var alertifyWrapper = new AlertifyWrapper(); >> alertifyWrapper.success(message); or alertifyWrapper*.error(message); >> *with appropriate (alertify.success/alertify.error..) >> - Modified test case written for alertify wrapper. >> >> >> Please review it and if it looks good then I'll commit the code. >> > Murtuza, > > Please review it. > > -- Thanks, Ashesh > >> >>> -- >>> Dave Page >>> Blog: http://pgsnake.blogspot.com >>> Twitter: @pgsnake >>> >>> EnterpriseDB UK: http://www.enterprisedb.com >>> The Enterprise PostgreSQL Company >>> >> >> >> >> -- >> *Akshay Joshi* >> *Principal Software Engineer * >> >> >> >> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246* >> > >
