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 1f3IXT-0004JX-PR for pgadmin-hackers@arkaria.postgresql.org; Tue, 03 Apr 2018 09:47:39 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1f3IXS-0005Ss-Hq for pgadmin-hackers@arkaria.postgresql.org; Tue, 03 Apr 2018 09:47:38 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1f3IXS-0005Si-BR for pgadmin-hackers@lists.postgresql.org; Tue, 03 Apr 2018 09:47:38 +0000 Received: from mail-ot0-x243.google.com ([2607:f8b0:4003:c0f::243]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1f3IXL-0002Kh-Hh for pgadmin-hackers@postgresql.org; Tue, 03 Apr 2018 09:47:37 +0000 Received: by mail-ot0-x243.google.com with SMTP id m22-v6so18663089otf.10 for ; Tue, 03 Apr 2018 02:47:30 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=enterprisedb-com.20150623.gappssmtp.com; s=20150623; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=l6/ElYNrUcS/zU9VRPXBpQ4kvBUHBRW2Gs6sE3gppq8=; b=tFeDjyIAkK+mQ1IVVsfKXErNgc1Au60gBHbp4BO3cVeJf1xwm2f+Uvf7bF2+qR3x2e DhazfTLQlgyAlcnBtc97Bomt00mTY7z09Chiu1eDdmwHpHfzdZQqC6E/Kcv+qiJwpnyN Td3SX+QQcOnRYks3uivzMb05007f7P7AyIA+oz8CIt3k4cMFSo4Hxp0et/IaFxW1aBIU FEo6oMDbakW6zWfASZRsoZDoSuwVo+ewR6vbd2/55wegsV37CtBjcnyIRpWzFfkU//Ww v3Gz46HmoolmKjHy2rXQIb3wqgrbUMlo5pQCqzsRPZLGjCeVr4IVjEji+INVnLwio3+B p0lw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=l6/ElYNrUcS/zU9VRPXBpQ4kvBUHBRW2Gs6sE3gppq8=; b=Y6blyeh11H/XKkw2DrZBZ2s3k/iWyfs6av9G8Tc+7Ha4iOjUj7wf2ibfTgWy/tbN1R 8JeMUGGwhZK8P35QF8JqykgrB071SBYRInNoaoEB7RnDEfn8gB78r8QOg97pF9vYkA4w vnn4UIFwpwiMQtReu4VyKwxJQLXUf6qUPvebdp3wRYM9plKol05HYxna84hP2x5N0+H7 as1oad34rQuQ7d49bTBm0nkUz4F02q60UqB9jUJMb64eFxL54RlpxQTRBS0x1DBZ4FqG gW2lw+iz6AglryLWFDtyXwr7uz3W3EejuSfmybD83Q8DxZeZ/osWcHDnRL1fhJHi3wec bwvQ== X-Gm-Message-State: ALQs6tBnHrPPalOdnWytcUcB/j4Ybtd5mvfXNLuOcPEFxLPKQJol86do iyT1o8MaL2yuK2fjgR++319ItudX8YTMmdutUJvYGw== X-Google-Smtp-Source: AIpwx4+RkyOTWEkHeb6zE0PFl3ZadlNUlSMQoSwXCvsE/BYDwxN551+MaMIqq1l5rhGPvydX6ylgYimSEBjROdYICdU= X-Received: by 2002:a9d:400d:: with SMTP id m13-v6mr3500965ote.391.1522748849186; Tue, 03 Apr 2018 02:47:29 -0700 (PDT) MIME-Version: 1.0 Received: by 10.74.138.251 with HTTP; Tue, 3 Apr 2018 02:47:28 -0700 (PDT) In-Reply-To: References: From: Khushboo Vashi Date: Tue, 3 Apr 2018 15:17:28 +0530 Message-ID: Subject: Re: [pgAdmin4][Patch]: RM #1978 - Add an option to allow user to disable alertifyjs and acitree animations To: Joao De Almeida Pereira Cc: Dave Page , pgadmin-hackers Content-Type: multipart/alternative; boundary="0000000000003d09130568ee9844" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --0000000000003d09130568ee9844 Content-Type: text/plain; charset="UTF-8" Hi Joao, Thanks for the review. I have sent the patch. On Thu, Mar 29, 2018 at 8:46 PM, Joao De Almeida Pereira < jdealmeidapereira@pivotal.io> wrote: > 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: > > Fixed > . Why does modify_animation.js have a dependency on sources/pgadmin if it > doesnt use it? > Removed, it was by mistake > . Can we convert modify_animation.js to ES6 without requirejs? > Done > . Why does modifyAnimation function have 2 arguments if we never use the > second one? > Not applicable now as it has been changed. > . Can we convert modify_animation_spec.js to ES6 without requirejs? > Done > . Why is pgBrowser.tree.options function called 4 times in the tests? > While setting tree options, it has been used 4 times. > As an aside, when you use toHaveBeenCalledWith it is redundant to > expect on toHaveBeenCalled > Thanks for the information > . Looks like we are missing some coverage of the alertify modification as > well > I have improved the coverage. > > > 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.... > > Will check and fix. > 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 > > Thanks, Khushboo > 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 >> > --0000000000003d09130568ee9844 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Joao,

Thanks for the review. I have = sent the patch.

On Thu, Mar 29, 2018 at 8:46 PM, Joao De Almeida Pereira <jde= almeidapereira@pivotal.io> wrote:
Hi Khushboo,

The patch looks lik= e it is heading on the correct direction, we passed it through our test pip= eline and all tests passed.

We found the same issu= es as Dave mentioned in his email, also after some code review we have the = following questions/comments:

Fixed=C2=A0
. Why does modify_animation.js have a dependency on sources/p= gadmin if it doesnt use it?
Removed, it = was by mistake=C2=A0
<= div>
. Can we convert modify_animation.js to ES6 without requirejs?
Done=C2=A0
. Why does=C2=A0modifyAnimation function hav= e 2 arguments if we never use the second one?
Not applicable now as it has been changed.
. Can we convert modify_animation_spec= .js to ES6 without requirejs?
Done=C2=A0=
. Why is pg= Browser.tree.options function called 4 times in the tests?
While setting tree options, it has been used 4 times.
=C2=A0 =C2=A0A= s an aside, when you use=C2=A0toHaveBeenCalledWith it is redundant to expec= t on=C2=A0toHaveBeenCalled
Thanks for th= e information=C2=A0
. Looks like we are missing some coverage of the alertify modificat= ion as well
I have improved t= he coverage.
=C2=A0

As an aside get_preferen= ces, the "cache", is still broken, if the cache as no value it wi= ll retrieve it but returns undefined to the caller. This behavior need to b= e addressed. We should change get preferences to be a Promise based thing o= r else this might become a problem....

Will check and fix.
As another aside, one of our goals should = be to move away from requirejs into a full ES6, webpack javascript build. I= n 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
T= hanks,
Khushboo=C2=A0
On Thu, Mar 29, 2018 at 9:25 AM Dave Page <dpage@pgadmin.org> wrote:
Hi
=
On Thu, Mar 29, 2018 at 1:51 P= M, Khushboo Vashi <khushboo.vashi@enterprisedb.com&= gt; wrote:


= On Mon, Mar 26, 2018 at 6:07 PM, Dave Page <= ;dpage@pgadmin.org> wrote:
<= div dir=3D"ltr">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 us= er 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 mat= ter of personal taste and circumstance.

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

I found some issues I'm afraid:<= /div>

- The label "Enable dialogues/notifications a= nimation?" should read "Enable dialogue/notification animation?&q= uot;

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

=C2=A0- After di= sabling dialogue/notification animations, I cannot re-enable notification a= nimations. If I flip the switch back on, dialogue animations immediately st= art working again, but notification animations don't even work followin= g a reload.

--0000000000003d09130568ee9844--