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@pgadmin.org> wrote:
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.
