Re: Discussion on improving alertify notifications logic

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*
>