Postgres Pro
Downloads
Home   >   mailing lists

Thread: Discussion on improving alertify notifications logic

Discussion on improving alertify notifications logic

From
Akshay Joshi
Date:
Hi All

As in commit "Update alertify alerts to use the styling defined in the style guide":


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.

--
Akshay Joshi
Principal Software Engineer 


Phone: +91 20-3058-9517
Mobile: +91 976-788-8246
Attachment

Re: Discussion on improving alertify notifications logic

From
Sarah McAlear
Date:
Hi Akshay!

That seems like a good idea. When we're using the extended pgadmin.alerrtify in the codebase it would be beneficial to call it pgAdminAlertify or something similar so that it is clear that it isn't only the alertify library, but it's been extended.

Rob & Sarah 

On Thu, Jul 27, 2017 at 9:41 PM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
Hi All

As in commit "Update alertify alerts to use the styling defined in the style guide":


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.

--
Akshay Joshi
Principal Software Engineer 



Re: Discussion on improving alertify notifications logic

From
Akshay Joshi
Date:
Hi Sara

On Fri, Jul 28, 2017 at 9:19 AM, Sarah McAlear <smcalear@pivotal.io> wrote:
Hi Akshay!

That seems like a good idea. When we're using the extended pgadmin.alerrtify in the codebase it would be beneficial to call it pgAdminAlertify or something similar so that it is clear that it isn't only the alertify library, but it's been extended.

   Thanks, but If I understand to you correctly calling it pgAdminAlertify will required code changes wherever alertify.<method> is called.

Rob & Sarah 

On Thu, Jul 27, 2017 at 9:41 PM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
Hi All

As in commit "Update alertify alerts to use the styling defined in the style guide":


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.

--
Akshay Joshi
Principal Software Engineer 






--
Akshay Joshi
Principal Software Engineer 


Phone: +91 20-3058-9517
Mobile: +91 976-788-8246

Re: Discussion on improving alertify notifications logic

From
Dave Page
Date:


On Thu, Jul 27, 2017 at 2:41 PM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
Hi All

As in commit "Update alertify alerts to use the styling defined in the style guide":


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.
 
--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Discussion on improving alertify notifications logic

From
Akshay Joshi
Date:
Hi All

On Fri, Jul 28, 2017 at 1:51 PM, Dave Page <dpage@pgadmin.org> wrote:


On Thu, Jul 27, 2017 at 2:41 PM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
Hi All

As in commit "Update alertify alerts to use the styling defined in the style guide":


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.  
 
--
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-9517
Mobile: +91 976-788-8246
Attachment

Re: Discussion on improving alertify notifications logic

From
Ashesh Vashi
Date:
On Mon, Jul 31, 2017 at 2:54 PM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
Hi All 

On Fri, Jul 28, 2017 at 1:51 PM, Dave Page <dpage@pgadmin.org> wrote:


On Thu, Jul 27, 2017 at 2:41 PM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
Hi All

As in commit "Update alertify alerts to use the styling defined in the style guide":


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-9517
Mobile: +91 976-788-8246

Re: Discussion on improving alertify notifications logic

From
Murtuza Zabuawala
Date:
Hi,

Looks good to me.

Regards,
Murtuza
On Mon, Jul 31, 2017 at 2:59 PM, Ashesh Vashi <ashesh.vashi@enterprisedb.com> wrote:
On Mon, Jul 31, 2017 at 2:54 PM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
Hi All 

On Fri, Jul 28, 2017 at 1:51 PM, Dave Page <dpage@pgadmin.org> wrote:


On Thu, Jul 27, 2017 at 2:41 PM, Akshay Joshi <akshay.joshi@enterprisedb.com> wrote:
Hi All

As in commit "Update alertify alerts to use the styling defined in the style guide":


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-9517
Mobile: +91 976-788-8246