Hi Joao, Thank you for your time in reviewing the patch.
On Wed, Feb 21, 2018 at 9:11 PM, Joao De Almeida Pereira < jdealmeidapere...@pivotal.io> wrote: > Hello Murtuza, > After reviewing the code I have some suggestions: > - We should split the PEP and the features into different patches, or > else it becomes very hard to separate Feature code from Code Style change > I'll keep that in mind next time onwards. > - The function: `getTextRepresentaionShortcut` has a typo > I'll fix that. > - As a personal point I have a hard time reading multiple declarations of > variables in javascript under a single `var`, I prefer 1 `let` per variable > - I think we should use cameCase for our variables in Javascript code > - What is the reason behind the move of the key shortcuts back into the > SQLEditorController? This look like something that could be extracted from > SQLEditor file into its own > I'll try to move that code. > - Looks like we are missing some test coverage on the new implemented > parts > I didn't added any new code as such, I just moved out preferences code out from main file. > - The getKeyboardShortcuts function is retrieving information from > window.top and window.opener I think that we can come up with a better > pattern then this. Relying on window information doesn't look very good. > That was the pattern for JQuery that the new frameworks are trying to avoid > because polluting the global scope is almost always a Code Smell. > What do you suggest on this? > > > I see there was some refactoring on the backend with the creation of > RegisterQueryToolPreferences and it is looking good, hope that we can do > this in more places specially in the front-end. > > Using the patch that you sent I passed it through our CI and everything is > Passing. > > Thanks > Joao > > On Tue, Feb 20, 2018 at 1:13 PM Murtuza Zabuawala <murtuza.zabuawala@ > enterprisedb.com> wrote: > >> Hi, >> >> PFA patch for adding accessibility features in query tool. >> RM#2900 >> >> Please review. >> >> -- >> Regards, >> Murtuza Zabuawala >> EnterpriseDB: http://www.enterprisedb.com >> The Enterprise PostgreSQL Company >> >>
