public inbox for [email protected]
help / color / mirror / Atom feedFrom: Joao De Almeida Pereira <[email protected]>
To: Khushboo Vashi <[email protected]>
Cc: Dave Page <[email protected]>
Cc: pgadmin-hackers <[email protected]>
Subject: Re: [pgAdmin4][Patch]: RM #1978 - Add an option to allow user to disable alertifyjs and acitree animations
Date: Thu, 29 Mar 2018 15:16:36 +0000
Message-ID: <CAE+jjamViJ=nF9PK+Yyvk8+yTMwQ5VMoeq=gTGnfE-Qo1ek=fg@mail.gmail.com> (raw)
In-Reply-To: <CA+OCxoyp9smBhDexiWeCOcp4H8PAwnpkHnmg2ojZDtOSAFd=qw@mail.gmail.com>
References: <CAFOhELc-ST96E35Wcih=uXuXYe5ahnb4Qp+WWAYFPDBP1XTr=g@mail.gmail.com>
<CA+OCxow84Sigyn=2QQj4RKEV8ManXNqrg4kSmvhxgvbiFn8cpw@mail.gmail.com>
<CAFOhELc21et_BLjzrcro8_tk5-EDfHYP-cmuhST2s9wzR5o9ng@mail.gmail.com>
<CA+OCxoyp9smBhDexiWeCOcp4H8PAwnpkHnmg2ojZDtOSAFd=qw@mail.gmail.com>
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 <[email protected]> wrote:
> Hi
>
> On Thu, Mar 29, 2018 at 1:51 PM, Khushboo Vashi <
> [email protected]> wrote:
>
>>
>>
>> On Mon, Mar 26, 2018 at 6:07 PM, Dave Page <[email protected]> wrote:
>>
>>> Hi
>>>
>>> On Mon, Mar 26, 2018 at 7:23 AM, Khushboo Vashi <
>>> [email protected]> 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
>
view thread (12+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: [pgAdmin4][Patch]: RM #1978 - Add an option to allow user to disable alertifyjs and acitree animations
In-Reply-To: <CAE+jjamViJ=nF9PK+Yyvk8+yTMwQ5VMoeq=gTGnfE-Qo1ek=fg@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox