Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1f1ZIM-0005OB-Lb for pgadmin-hackers@arkaria.postgresql.org; Thu, 29 Mar 2018 15:16:54 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1f1ZIL-0004SI-DU for pgadmin-hackers@arkaria.postgresql.org; Thu, 29 Mar 2018 15:16:53 +0000 Received: from makus.postgresql.org ([2001:4800:1501:1::229]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1f1ZIL-0004RZ-0F for pgadmin-hackers@lists.postgresql.org; Thu, 29 Mar 2018 15:16:53 +0000 Received: from mail-io0-x236.google.com ([2607:f8b0:4001:c06::236]) by makus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1f1ZIG-00087y-68 for pgadmin-hackers@postgresql.org; Thu, 29 Mar 2018 15:16:50 +0000 Received: by mail-io0-x236.google.com with SMTP id d5so8064704iob.9 for ; Thu, 29 Mar 2018 08:16:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=pivotal-io.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=obClxfM5BtqbPmLqewhfiLH7g0/dJwotWvZeVwiIsIg=; b=DKvRjWGRyy1Kibl0WBu7BYMBnPbJXMzyZTg9fPFcT/2XJ5jHGY38AUFMov9C+aEnGL 2C2ARpuJlxmssEW19fjo/zD3sgQ743UHILDUqDd3yJh+teajPRUvQ+uDa4dhmjCXkwcm Cw9uwJ6aVS1emWeMSySCtKwalw1AjCCjqGfjYxpFzhGsM7jzHtEjOBaZ+PibtwXmkY+s aRKufw+s8Sjajo70Ag9u83ZCBp2QxVdOKyrzDzN+A8qPGE+1oPp9Q1sLYZrEVCN55Kiy wqK09rMMEv64QwLwR1gAMRYDGCch/qOGwvwz8iA1f0iwi+UEOhr0xW7/R8MGLCHZB6GV UPQw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=obClxfM5BtqbPmLqewhfiLH7g0/dJwotWvZeVwiIsIg=; b=WivLmP4kOoRmXtm6sMeD3ANTygA0raMknp/Bs8MPWsSWNWXrH+HtCSB6BPsS9oz2M2 2Fp6HTW03uldp8iEm6exL06Ah4SLTpT2qgjbEACfAf9bMeiXCEZDTpymN8Ke5KNVmfBd AmNfQwj9+XJs6azKXYk9YsnTIQ5UbPVVAnb5P7c7r5lMOw96Ux/XtAVL6XYajI2m284N 5QU7PZYmLawkj/UAZDu4xTre9J+SgeIs8gSCEUOVo1y6znP78OCL5tbN7HiOLGJYfgcv oewGjM11cfnGQ6J8kB73bt/tT8QEJ9WJDUWCijl8Wc/qkt85+Bi+5vLqhDoQESEmw35p uVsQ== X-Gm-Message-State: AElRT7FbfwZfIGyFG0LkBgkq97ZtR+2vAM2Q7JhbSKWEV0vV7j7SMirx 61BNAc7M/Fozjhx5ddiuSRft63n27Kp0e6FSZJTF/A== X-Google-Smtp-Source: AIpwx4+YaHBoUYwvTHrYFbVw/UwGBIcClkT+vccE4ANmP6TEYxkuOZawrOPDvnetyNZVotNFq+U/cl239iNUz/v6GCM= X-Received: by 10.107.143.211 with SMTP id r202mr21495844iod.185.1522336606831; Thu, 29 Mar 2018 08:16:46 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Joao De Almeida Pereira Date: Thu, 29 Mar 2018 15:16:36 +0000 Message-ID: Subject: Re: [pgAdmin4][Patch]: RM #1978 - Add an option to allow user to disable alertifyjs and acitree animations To: Khushboo Vashi Cc: Dave Page , pgadmin-hackers Content-Type: multipart/alternative; boundary="94eb2c05a4bcae05be05688e9ccd" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --94eb2c05a4bcae05be05688e9ccd Content-Type: text/plain; charset="UTF-8" Hi Khushboo, The patch looks like it is heading on the correct direction, we passed it through our test pipeline and all tests passed. We found the same issues as Dave mentioned in his email, also after some code review we have the following questions/comments: . Why does modify_animation.js have a dependency on sources/pgadmin if it doesnt use it? . Can we convert modify_animation.js to ES6 without requirejs? . Why does modifyAnimation function have 2 arguments if we never use the second one? . Can we convert modify_animation_spec.js to ES6 without requirejs? . Why is pgBrowser.tree.options function called 4 times in the tests? As an aside, when you use toHaveBeenCalledWith it is redundant to expect on toHaveBeenCalled . Looks like we are missing some coverage of the alertify modification as well As an aside get_preferences, the "cache", is still broken, if the cache as no value it will retrieve it but returns undefined to the caller. This behavior need to be addressed. We should change get preferences to be a Promise based thing or else this might become a problem.... As another aside, one of our goals should be to move away from requirejs into a full ES6, webpack javascript build. In order to do that we should try to write the least amount of code possible using requirejs syntax. If we really need to write something in requirejs let it be a wrapper that call our ES6 function/class Thanks Joao On Thu, Mar 29, 2018 at 9:25 AM Dave Page wrote: > Hi > > On Thu, Mar 29, 2018 at 1:51 PM, Khushboo Vashi < > khushboo.vashi@enterprisedb.com> wrote: > >> >> >> On Mon, Mar 26, 2018 at 6:07 PM, Dave Page wrote: >> >>> Hi >>> >>> On Mon, Mar 26, 2018 at 7:23 AM, Khushboo Vashi < >>> khushboo.vashi@enterprisedb.com> wrote: >>> >>>> Hi, >>>> >>>> Please find the attached patch to fix RM #1978: Add an option to allow >>>> user to disable alertifyjs and acitree animations. >>>> >>> >>> I think these really need to be per-user settings, not >>> per-installation.. Whether or not animations are shown is really a matter >>> of personal taste and circumstance. >>> >>> Right, it should be per-user settings. Please find the attached updated >> patch. >> > > I found some issues I'm afraid: > > - The label "Enable dialogues/notifications animation?" should read > "Enable dialogue/notification animation?" > > - Disabling treeview animation only seems to affect the main browser > treeview, and not others in the application (e.g. the one on the > Preferences panel). > > - After disabling dialogue/notification animations, I cannot re-enable > notification animations. If I flip the switch back on, dialogue animations > immediately start working again, but notification animations don't even > work following a reload. > > Thanks. > > -- > Dave Page > Blog: http://pgsnake.blogspot.com > Twitter: @pgsnake > > EnterpriseDB UK: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > --94eb2c05a4bcae05be05688e9ccd Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Khushboo,

The patch looks like it is= heading on the correct direction, we passed it through our test pipeline a= nd all tests passed.

We found the same issues as D= ave mentioned in his email, also after some code review we have the followi= ng questions/comments:

. Why does modify_animation.j= s have a dependency on sources/pgadmin if it doesnt use it?
. Can= we convert modify_animation.js to ES6 without requirejs?
. Why d= oes=C2=A0modifyAnimation function have 2 arguments if we never use the seco= nd one?
. Can we convert modify_animation_spec.js to ES6 without = requirejs?
. Why is pgBrowser.tree.options function called 4 time= s in the tests?
=C2=A0 =C2=A0As an aside, when you use=C2=A0toHav= eBeenCalledWith it is redundant to expect on=C2=A0toHaveBeenCalled
. Looks like we are missing some coverage of the alertify modification as= well


As an aside get_preferences, = the "cache", is still broken, if the cache as no value it will re= trieve it but returns undefined to the caller. This behavior need to be add= ressed. We should change get preferences to be a Promise based thing or els= e this might become a problem....

As another aside= , one of our goals should be to move away from requirejs into a full ES6, w= ebpack javascript build. In order to do that we should try to write the lea= st amount of code possible using requirejs syntax. If we really need to wri= te something in requirejs let it be a wrapper that call our ES6 function/cl= ass

Thanks
Joao

On Thu, Mar 29, 2018 at 9:25 AM Dave Page <= dpage@pgadmin.org> wrote:
Hi

On= Thu, Mar 29, 2018 at 1:51 PM, Khushboo Vashi <khushboo.vash= i@enterprisedb.com> wrote:


On Mon, Mar 26, 2018 at 6:07 PM, Dave Page &= lt;dpage@pgadmin.org= > wrote:
Hi

On Mon, Mar 26, 2018 at 7:23 AM, Khushboo Vashi <khushboo.vashi@enterprisedb.com> wrote:
Hi,

P= lease find the attached patch to fix RM #1978: Add an option to allow user = to disable alertifyjs and acitree animations.
=
I think these really need to be per-user settings, no= t per-installation.. Whether or not animations are shown is really a matter= of personal taste and circumstance.

=
Right, it should be per-user settings.=C2=A0 Pleas= e find the attached updated patch.=C2=A0

I found some issues I'm afraid:

- The label "Enable dialogues/notifications anim= ation?" should read "Enable dialogue/notification animation?"= ;

- Disabling treeview animation only seems to aff= ect the main browser treeview, and not others in the application (e.g. the = one on the Preferences panel).

=C2=A0- After disab= ling dialogue/notification animations, I cannot re-enable notification anim= ations. If I flip the switch back on, dialogue animations immediately start= working again, but notification animations don't even work following a= reload.

Thanks.
<= div>
--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company<= br>
--94eb2c05a4bcae05be05688e9ccd--