Re: [pgAdmin4][RM#2900] Adding accessibility features in query tool

Hi Joao,

Thank you for your time in reviewing the patch.

On Wed, Feb 21, 2018 at 9:11 PM, Joao De Almeida Pereira <
jdealmeidapereira(at)pivotal(dot)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
>>
>>