public inbox for [email protected]
help / color / mirror / Atom feedFrom: Khushboo Vashi <[email protected]>
To: Joao De Almeida Pereira <[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: Tue, 3 Apr 2018 15:17:28 +0530
Message-ID: <CAFOhELcaxZ1m=W-duHsd1iqkiS-sn9HmsJZ6kSMM-_FL5JweLw@mail.gmail.com> (raw)
In-Reply-To: <CAE+jjamViJ=nF9PK+Yyvk8+yTMwQ5VMoeq=gTGnfE-Qo1ek=fg@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>
<CAE+jjamViJ=nF9PK+Yyvk8+yTMwQ5VMoeq=gTGnfE-Qo1ek=fg@mail.gmail.com>
Hi Joao,
Thanks for the review. I have sent the patch.
On Thu, Mar 29, 2018 at 8:46 PM, Joao De Almeida Pereira <
[email protected]> 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 <[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: <CAFOhELcaxZ1m=W-duHsd1iqkiS-sn9HmsJZ6kSMM-_FL5JweLw@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