Re: Discussion on improving alertify notifications logic

Hi,

Looks good to me.

Regards,
Murtuza
On Mon, Jul 31, 2017 at 2:59 PM, Ashesh Vashi <ashesh(dot)vashi(at)enterprisedb(dot)com
> wrote:

> On Mon, Jul 31, 2017 at 2:54 PM, Akshay Joshi <
> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>
>> Hi All
>>
>
>> On Fri, Jul 28, 2017 at 1:51 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>
>>>
>>>
>>> On Thu, Jul 27, 2017 at 2:41 PM, Akshay Joshi <
>>> akshay(dot)joshi(at)enterprisedb(dot)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*
>>
>
>