Re: [pgAdmin4][Patch]: RM #1978 - Add an option to allow user to disable alertifyjs and acitree animations

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(at)pivotal(dot)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 <dpage(at)pgadmin(dot)org> wrote:
>
>> Hi
>>
>> On Thu, Mar 29, 2018 at 1:51 PM, Khushboo Vashi <
>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>
>>>
>>>
>>> On Mon, Mar 26, 2018 at 6:07 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>
>>>> Hi
>>>>
>>>> On Mon, Mar 26, 2018 at 7:23 AM, Khushboo Vashi <
>>>> khushboo(dot)vashi(at)enterprisedb(dot)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
>>
>