Hi Joao, Thanks for the review. I have sent the patch.
On Thu, Mar 29, 2018 at 8:46 PM, Joao De Almeida Pereira < jdealmeidapere...@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 <dp...@pgadmin.org> wrote: > >> Hi >> >> On Thu, Mar 29, 2018 at 1:51 PM, Khushboo Vashi < >> khushboo.va...@enterprisedb.com> wrote: >> >>> >>> >>> On Mon, Mar 26, 2018 at 6:07 PM, Dave Page <dp...@pgadmin.org> wrote: >>> >>>> Hi >>>> >>>> On Mon, Mar 26, 2018 at 7:23 AM, Khushboo Vashi < >>>> khushboo.va...@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 >> >
